Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Platform Agnostic TM Query #3846

Merged
merged 2 commits into from
May 15, 2019
Merged

Add Platform Agnostic TM Query #3846

merged 2 commits into from
May 15, 2019

Conversation

dsouzai
Copy link
Contributor

@dsouzai dsouzai commented May 13, 2019

In Eclipse OpenJ9, I need to add an AOT feature flag for Transactional Memory (TM) in order to ensure consistency with regard to TM across the compile and load runs. However, as it turns out, the processor query for determining whether TM is enabled is different for each platform. If it was as simple as different names, I would've just #ifdef'd the different queries and moved on. However, on x86, the query isn't even in the TR::CPU class; it's initialized as a static member of the x86 codegen class, and the initialize method is private that can only be accessed by the x86 codegen class.

This PR only contains changes that provide (for the most part) a wrapper around existing TM queries. The right thing to do would be to refactor all the code into the TR::CPU class, but that's a rabbit hole unto itself. I figure this change should suffice for now, until someone re-architects the code.

@dsouzai
Copy link
Contributor Author

dsouzai commented May 13, 2019

@0xdaryl @Leonardo2718 could you guys please review?

fjeremic
fjeremic reviewed May 13, 2019
View reviewed changes
compiler/z/env/OMRCPU.hpp Outdated
* Determines whether the Transactional Memory (TM) facility is available on the current processor.
* Alias of getSupportsTransactionalMemoryFacility() as a platform agnostic query.
*/
bool supportsTM();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of the duplication here, but until all the codegens have migrated to using the CPU class for CPU related queries we don't have a choice.

@dsouzai
Copy link
Contributor Author

dsouzai commented May 13, 2019 •
edited
Loading

I'm not really sure why the compile is failing; it's failing with

../../compiler/z/env/OMRCPU.cpp:213:17: error: member access into incomplete
      type 'TR::CPU'
   return self()->getSupportsTransactionalMemoryFacility();
                ^
../../compiler/env/OMRCPU.hpp:36:22: note: forward declaration of 'TR::CPU'
namespace TR { class CPU; }
                     ^

Why isn't the type of self() a complete type at this point? It's a .cpp file..

@dsouzai
Copy link
Contributor Author

dsouzai commented May 13, 2019

Another question, if the job was

RUN_LINT=yes RUN_BUILD=no SPEC=linux_x86-64 PLATFORM=amd64-linux64-gcc LLVM_CONFIG=llvm-config-3.8 CLANG=clang++-3.8 CXX_PATH=clang++-3.8 CXX=clang++-3.8

Why is the linter build also building p and z?

@0xdaryl
Copy link
Contributor

0xdaryl commented May 13, 2019

If you're adding a new CPU query, why not give it a more descriptive name while you're at it? e.g., supportsTransactionalMemoryInstructions (or something). It can't be queried in that many places in the compiler so we can afford the readability of an expanded name.

@0xdaryl
Copy link
Contributor

0xdaryl commented May 13, 2019

Rabbit holes are what refactoring in this project are all about. However, the more refactoring we all do the fewer of them there are. Some aren't as deep as you think they might be. The project appreciates any effort you (or anyone else) makes toward improving the code.

If you're not willing to take the plunge, could you open an issue in the OMR repo to track the proper refactoring effort that the eventual "someone" will need to take?

@Leonardo2718
Copy link
Contributor

Leonardo2718 commented May 13, 2019 •
edited
Loading

@dsouzai

I'm not really sure why the compile is failing

I believe the failure is because compiler/z/env/OMRCPU.cpp includes OMRCPU.hpp instead of CPU.hpp.

Why is the linter build also building p and z?

The linter is capable of running in a "cross-linting" mode so that errors can be detected on non-x86 platforms without having to build clang on those platforms.

Change absolute include to relative
7c6ddd3 
in z/env/OMRCPU.cpp, compiler/z/env/OMRCPU.hpp was being included.
This resulted in linter issues, because self() did not produce a
complete type. This commit replaces the absolute include path with a
relative include path, to allow the I-Path magic to do its thing.

Signed-off-by: Irwin D'Souza <[email protected]>
@dsouzai
Copy link
Contributor Author

dsouzai commented May 13, 2019

I believe the failure is because compiler/z/env/OMRCPU.cpp includes OMRCPU.hpp instead of CPU.hpp.

Thanks @Leonardo2718, that seemed to do the trick.

@dsouzai
Copy link
Contributor Author

dsouzai commented May 13, 2019 •
edited
Loading

If you're adding a new CPU query, why not give it a more descriptive name while you're at it? e.g., supportsTransactionalMemoryInstructions

@0xdaryl I changed the query to be supportsTransactionalMemoryInstructions . I'll open I opened an issue regarding the refactoring work (#3848).

@dsouzai dsouzai mentioned this pull request May 13, 2019
Closed
@dsouzai
Copy link
Contributor Author

dsouzai commented May 13, 2019

@fjeremic could you re-review, considering the change to the include path.

@dsouzai dsouzai mentioned this pull request May 13, 2019
Merged
@dsouzai
Copy link
Contributor Author

dsouzai commented May 14, 2019

@0xdaryl could you please re-review and kick off tests if everything looks good to you?

@0xdaryl
Copy link
Contributor

0xdaryl commented May 14, 2019

@genie-omr build all

@0xdaryl 0xdaryl self-assigned this May 14, 2019
0xdaryl
0xdaryl reviewed May 14, 2019
View reviewed changes
compiler/env/OMRCPU.hpp
@@ -131,6 +131,8 @@ class CPU
bool isI386() { return _minorArch == TR::m_arch_i386; }
bool isAMD64() { return _minorArch == TR::m_arch_amd64; }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API should have the same Doxygen comment as the platform-specific ones. It is the API used by architectures that don't have TM support yet (e.g., ARM32, AArch64).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, missed that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated (see https://github.com/eclipse/omr/compare/9142f82165c1480c850be7146abc18c6ecc4fd46..0193bf0dfe329515a899e6ef22c35ef6fcae37ec)

Add Platform Agnostic TM Query
0193bf0 
This commit adds a wrapper API around existing Transactional Memory (TM)
queries in order to have a single query that is common across all
platforms.

Signed-off-by: Irwin D'Souza <[email protected]>
@0xdaryl
Copy link
Contributor

0xdaryl commented May 14, 2019

@genie-omr build all

@0xdaryl 0xdaryl merged commit e4ae0f5 into eclipse-omr:master May 15, 2019
@dsouzai dsouzai deleted the tmQuery branch January 4, 2021 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xdaryl 0xdaryl 0xdaryl approved these changes

@fjeremic fjeremic fjeremic approved these changes

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

Assignees

@0xdaryl 0xdaryl

Labels
arch:power arch:x86 arch:z comp:compiler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dsouzai @0xdaryl @Leonardo2718 @fjeremic