Allow extensions to GlobalRegisterAllocator #6577

kevindean12 · 2022-06-20T11:34:33Z

Splits TR_RegisterCandidate, TR_RegisterCandidates, and GlobalSet into OMR and TR namespaces to allow downstream projects to provide derived classes that extend their functionality. Makes TR_GlobalRegister a proper extensible class, which is necessary because the way that TR_GlobalRegister is used requires that it does not have any virtual functions (instances are initialized inside of a TR_Array and memset to 0, which would interfere with the vtable if the class used dynamic polymorphism). Finally, makes some members of GlobalRegisterAllocator virtual and moves some members' access in GlobalRegisterAllocator from private to protected to allow downstream projects to create derived class extensions of GlobalRegisterAllocator.

kevindean12 · 2023-01-11T18:37:10Z

Jenkins build all

kevindean12 · 2023-01-12T13:23:11Z

Jenkins build all

0xdaryl · 2023-02-07T19:55:52Z

@hzongaro : please review

hzongaro

I think the changes look good overall. One question I had was whether you considered moving GlobalRegisterAllocator itself to the TR and OMR namespaces as well, and if not, why not?

As I don't have a lot of experience with the tradeoffs involved, I'm just wondering whether doing that now would cause problems for the kind of extension to GlobalRegisterAllocator that you're working on, or if some other downstream project required it in the future, what kind of difficulty it might cause for your project at that point.

hzongaro · 2023-02-28T18:22:55Z

compiler/optimizer/GlobalRegister_inlines.hpp

+
+#include "optimizer/OMRGlobalRegister_inlines.hpp"
+
+#endif


There's a newline missing at the end of this file.

Thanks for the review! I didn't strongly consider moving all of TR_GlobalRegisterAllocator to OMR/TR namespace, but that was basically only because it wasn't strictly needed to allow the extension points I was trying to enable here. That being said, I'm not opposed to it, and I don't think that if other projects wanted it structured that way it would cause any problems in downstream projects that I'm aware of.

My understanding (for what it's worth; others with more experience with the OMR code than I may have additional thoughts) is that the tradeoff there is largely a matter of whether making a change to the structure of the class in that way may inadvertently lead to new bugs/disruptions to existing projects, against the consistency it would afford, both to OMR itself in having a consistent style and to downstream projects in having a more consistent set of extension points for the various optimization classes, to have all of the optimizations structured in a similar way (which at the moment actually isn't consistent across the various optimizations - some are TR_, others are in the OMR/TR namespace, others are fully extensible classes - but probably it would be nice if it were consistently one or another of those options).

OK, that sounds reasonable. Once you have a chance to add the missing newline, I'll mark my review as approved.

I've added a newline to the end of the file.

hzongaro · 2023-02-28T18:39:08Z

compiler/optimizer/GlobalRegisterAllocator.hpp

+   int32_t             numberOfRegistersLiveOnEntry(TR_Array<TR::GlobalRegister> &, bool);
+   TR_PairedSymbols   *findOrCreatePairedSymbols(TR::SymbolReference *symRef1, TR::SymbolReference *symRef2);
+   bool                isDependentStore(TR::Node *, const TR_UseDefInfo::BitVector &, TR::SymbolReference *, bool *);
+   TR::TreeTop *        findPrevTreeTop(TR::TreeTop * &, TR::Node * &, TR::Block *, TR::Block *);


Did you really intend to make _visitCount and all the other fields that follow protected rather than leaving them private? I'm guessing "yes", but it's hard to know what might need to be accessed in downstream implementations.

I did intend to make those protected, yes.

hzongaro

Looks good to me

hzongaro · 2023-03-06T14:11:04Z

Jenkins build all

hzongaro · 2023-03-06T16:04:44Z

I just came across issue #3519, which describes the on-going goal of moving compiler APIs to the TR namespace. Just mentioning it as @0xdaryl requested in that issue.

hzongaro · 2023-03-09T21:38:50Z

@kevindean12, the copyright updates that you made now conflict with the changes to the new style of copyright from pull request #6923. It also looks like the CI builds never ran for some reason. Once you've removed your updates to the copyrights, I'll request the CI builds again.

-Put TR_RegisterCandidate, TR_RegisterCandidates, and GlobalSet classes into the OMR namespace and create derived classes in TR namespace, allowing downstream projects to add a project-level class to those hierarchies in order to extend functionality. -Make members of the above classes protected -Make the following methods virtual: -GlobalSet::collectReferencedAutoSymRefs() -RegisterCandidate::symbolIsLive() -RegisterCandidate::processLiveOnEntryBlocks() -RegisterCandidates::findOrCreate() -RegisterCandidates::find(TR::SymbolReference *) -RegisterCandidates::assign() -RegisterCandidates::computeAvailableRegisters()

kevindean12 · 2023-03-10T02:25:06Z

Rebased and resolved the copyrights conflicts.

kevindean12 · 2023-03-10T02:26:00Z

Jenkins build all

hzongaro · 2023-03-10T14:45:17Z

MacOS failure appears to be due to #6516 - or it could be the more recently reported #6924, if that's a distinct problem.

Daryl 0xdaryl, I think this change looks good to merge.

0xdaryl · 2023-03-14T11:06:37Z

@hzongaro : Henry, this will need a corresponding change to the makefiles in OpenJ9 to handle the rename of "OMRRegisterCandidate.cpp" and the addition of "OMRGlobalRegister.cpp"

These are the "common.mk" files used for the older SDK 8 builds.

If you could create a PR for that then I could do a coordinated merge of the two PRs. Thanks.

hzongaro · 2023-03-14T13:03:52Z

this will need a corresponding change to the makefiles in OpenJ9

@0xdaryl, thank you for catching that! I will take care of it.

OMR pull request eclipse/omr#6577 renames RegisterCandidate.cpp to OMRRegisterCandidate.cpp, and adds a new file, OMRGlobalRegister.cpp. Both of these files are located in omr/compiler/optimizer. This change updates the OpenJ9 common.mk file that is used by older versions of the build infrastructure to compile those files. Signed-off-by: Henry Zongaro <[email protected]>

hzongaro

Kevin @kevindean12, sorry for the late re-review comments!

hzongaro · 2023-03-14T13:42:37Z

fvtest/compilertest/build/files/common.mk

@@ -155,7 +155,7 @@ JIT_PRODUCT_BACKEND_SOURCES+=\
    $(JIT_OMR_DIRTY_DIR)/optimizer/ReachingDefinitions.cpp \
    $(JIT_OMR_DIRTY_DIR)/optimizer/OMRRecognizedCallTransformer.cpp \
    $(JIT_OMR_DIRTY_DIR)/optimizer/RedundantAsyncCheckRemoval.cpp \
-    $(JIT_OMR_DIRTY_DIR)/optimizer/RegisterCandidate.cpp \
+    $(JIT_OMR_DIRTY_DIR)/optimizer/OMRRegisterCandidate.cpp \


Daryl's comment prompted me to take a look at the changes to these common.mk files again. I believe a new entry will be needed for OMRGlobalRegister.cpp

Oops, thanks for catching it! I'll update these two common.mk files.

hzongaro · 2023-03-14T13:43:45Z

jitbuilder/build/files/common.mk

@@ -157,7 +157,7 @@ JIT_PRODUCT_BACKEND_SOURCES+=\
    $(JIT_OMR_DIRTY_DIR)/optimizer/ReachingDefinitions.cpp \
    $(JIT_OMR_DIRTY_DIR)/optimizer/OMRRecognizedCallTransformer.cpp \
    $(JIT_OMR_DIRTY_DIR)/optimizer/RedundantAsyncCheckRemoval.cpp \
-    $(JIT_OMR_DIRTY_DIR)/optimizer/RegisterCandidate.cpp \
+    $(JIT_OMR_DIRTY_DIR)/optimizer/OMRRegisterCandidate.cpp \


Daryl's comment prompted me to take a look at the changes to these common.mk files again. I believe a new entry will be needed for OMRGlobalRegister.cpp

-replace uses of TR_GlobalRegister with TR::GlobalRegister

-the following GRA methods are made virtual for downstream projects to be able to extend functionality: -markAutosUsedIn() -transformNode() -prepareForBlockExit() -registerIsLiveAcrossEdge() -make several GRA members protected -make TR_LiveRangeSplitter members protected Closes: eclipse#6529

kevindean12 · 2023-03-15T10:47:54Z

Jenkins build all

hzongaro · 2023-03-15T16:55:44Z

Thanks, Kevin @kevindean12. The one failure with the macOS build again appears to be due to #6924, so I think everything looks good.

Daryl @0xdaryl - FYI

kevindean12 requested review from fjeremic, aviansie-ben, 0xdaryl, knn-k, vijaysun-omr, Leonardo2718 and mstoodle as code owners June 20, 2022 11:34

github-actions bot added arch:aarch32 arch:aarch64 comp:compiler labels Jun 20, 2022

kevindean12 changed the title ~~Allow extensions to GlobalRegisterAllocator~~ WIP Allow extensions to GlobalRegisterAllocator Jun 20, 2022

kevindean12 force-pushed the extend-regcan branch 3 times, most recently from 7d17d38 to 31f5471 Compare June 21, 2022 20:20

kevindean12 changed the title ~~WIP Allow extensions to GlobalRegisterAllocator~~ Allow extensions to GlobalRegisterAllocator Jun 30, 2022

kevindean12 force-pushed the extend-regcan branch from 31f5471 to 144d051 Compare October 27, 2022 14:38

kevindean12 force-pushed the extend-regcan branch from 144d051 to 82ab93d Compare January 11, 2023 17:31

kevindean12 force-pushed the extend-regcan branch from 82ab93d to fa14415 Compare January 11, 2023 19:29

kevindean12 requested a review from charliegracie as a code owner January 11, 2023 19:29

kevindean12 force-pushed the extend-regcan branch from fa14415 to 887415d Compare January 12, 2023 10:51

hzongaro added enhancement arch:aarch32 arch:aarch64 arch:x86 arch:power and removed arch:aarch32 arch:aarch64 labels Feb 8, 2023

hzongaro added arch:z arch:riscv labels Feb 9, 2023

hzongaro reviewed Feb 28, 2023

View reviewed changes

kevindean12 force-pushed the extend-regcan branch 2 times, most recently from caffe97 to 492fd8a Compare March 3, 2023 16:04

hzongaro approved these changes Mar 6, 2023

View reviewed changes

0xdaryl self-assigned this Mar 6, 2023

kevindean12 force-pushed the extend-regcan branch from 492fd8a to 04ddf49 Compare March 10, 2023 02:07

hzongaro reviewed Mar 14, 2023

View reviewed changes

hzongaro mentioned this pull request Mar 14, 2023

Update common.mk to compile new OMR Register Allocator files eclipse-openj9/openj9#16912

Merged

kevindean12 added 2 commits March 14, 2023 17:42

Make TR_GlobalRegister an extensible class

a8787e4

-replace uses of TR_GlobalRegister with TR::GlobalRegister

kevindean12 force-pushed the extend-regcan branch from 04ddf49 to 7383012 Compare March 14, 2023 21:45

0xdaryl approved these changes Mar 16, 2023

View reviewed changes

0xdaryl merged commit 1a20c5d into eclipse:master Mar 20, 2023

hzongaro mentioned this pull request Mar 20, 2023

Reference openjdk.org instead of openjdk.java.net #6927

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow extensions to GlobalRegisterAllocator #6577

Allow extensions to GlobalRegisterAllocator #6577

kevindean12 commented Jun 20, 2022

kevindean12 commented Jan 11, 2023

kevindean12 commented Jan 12, 2023

0xdaryl commented Feb 7, 2023

hzongaro left a comment

hzongaro Feb 28, 2023

kevindean12 Mar 2, 2023

hzongaro Mar 2, 2023

kevindean12 Mar 6, 2023

hzongaro Feb 28, 2023

kevindean12 Mar 2, 2023

hzongaro left a comment

hzongaro commented Mar 6, 2023

hzongaro commented Mar 6, 2023

hzongaro commented Mar 9, 2023

kevindean12 commented Mar 10, 2023

kevindean12 commented Mar 10, 2023

hzongaro commented Mar 10, 2023 •

edited

Loading

0xdaryl commented Mar 14, 2023

hzongaro commented Mar 14, 2023

hzongaro left a comment

hzongaro Mar 14, 2023

kevindean12 Mar 14, 2023

hzongaro Mar 14, 2023

kevindean12 commented Mar 15, 2023

hzongaro commented Mar 15, 2023


		#include "optimizer/OMRGlobalRegister_inlines.hpp"

		#endif

Allow extensions to GlobalRegisterAllocator #6577

Allow extensions to GlobalRegisterAllocator #6577

Conversation

kevindean12 commented Jun 20, 2022

kevindean12 commented Jan 11, 2023

kevindean12 commented Jan 12, 2023

0xdaryl commented Feb 7, 2023

hzongaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzongaro left a comment

Choose a reason for hiding this comment

hzongaro commented Mar 6, 2023

hzongaro commented Mar 6, 2023

hzongaro commented Mar 9, 2023

kevindean12 commented Mar 10, 2023

kevindean12 commented Mar 10, 2023

hzongaro commented Mar 10, 2023 • edited Loading

0xdaryl commented Mar 14, 2023

hzongaro commented Mar 14, 2023

hzongaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindean12 commented Mar 15, 2023

hzongaro commented Mar 15, 2023

hzongaro commented Mar 10, 2023 •

edited

Loading