Changes in RegDepCopyRemoval for splitPostGRA block splitter #5219

r30shah · 2020-05-20T16:44:46Z

RegDepCopyRemoval is the last ran optimization which intends to reduce register shuffling due to global register dependencies. It was generating tree where we have a PassThrough Child under PassThrough to copy node to new virtual register. This tree was not working well with the post GRA block splitter. Issue was that PassThrough child encountered in GlRegDeps was anchored to tree top. In postGRA block splitter, it records the regStores encountered before the GlRegDeps in extended basic block to guide decision on which register to chose for the node after split point.In this commit, adding changes to both splitPostGRA block splitter and RegDepCopyRemoval optimization so that we can split the blocks after later has run.

Signed-off-by: Rahil Shah [email protected]

r30shah · 2020-05-20T16:52:23Z

@andrewcraik @Leonardo2718 Can I please get a review on these changes? I have added changes to RegDepCopyRemoval. So now instead of following tree,

n1n treetop
n2n    PassThrough 
n3n       node A
...
n4n    GlRegDeps
n5n       PassThrough Rx
n2n          => PassThrough

We will have following tree

n0n regStore Rx
n2n    PassThrough
n3n       node A
...
n4n    GlRegDeps
n5n       PassThrough Rx
n2n          => PassThrough

This will work well if we try to split the block after this opt has run. Marking this WIP while I am verifying that there is no perf impact of these changes.

andrewcraik · 2020-05-20T19:21:33Z

Change seems reasonable - will wait for unWIP and I'll continue the review then.

andrewcraik · 2020-08-27T14:38:54Z

@r30shah any further update on this?

r30shah · 2020-08-27T15:02:34Z

@andrewcraik Thanks for reminding me. I did fix the issue but could not get the large batch of performance runs due to no free time one machine. Let me book it and get the performance numbers.

r30shah · 2020-09-01T13:02:56Z

I ran a liberty Daytrader benchmark on x86 and here are the results (Only pasting relative performance of average throughput of 20 runs)

Baseline Run  - 100.00 %
RegDepCopyRemovalChanges - 100.11 %

@andrewcraik Changes does not impact the performance impact we have with the RegDepCopyRemoval optimization. Removed WIP. This one is good for review.

andrewcraik · 2020-09-01T21:07:33Z

compiler/optimizer/RegDepCopyRemoval.hpp

@@ -105,7 +106,7 @@ class RegDepCopyRemoval : public TR::Optimization
   NodeChoice &getNodeChoice(TR_GlobalRegisterNumber reg);

   void discardAllNodeChoices();
-   void discardNodeChoice(TR_GlobalRegisterNumber reg);
+   void discardNodeChoice(TR_GlobalRegisterNumber reg, bool clearRegStore = false);


I don't like this boolean flag - could we have discardNodeChoice and something like discardNodeChoiceAndClearStore or something like that? it will be easier to follow the logic rather than having a random bool flag.

As the only case when we need to clear up the Node Choice completely is when we start walking new block which is not extension, at that point it cleans up all the data structures that was filled for previous analysis. I renamed this to clearNodeChoice.

andrewcraik · 2020-09-01T21:10:57Z

@r30shah not sure what to make of the failures... could you have a look?

r30shah · 2020-09-01T21:27:53Z

@andrewcraik I believe it is failing because of this guy, https://github.com/eclipse/omr/blob/cd259de5afc5c80fdc42491c7201564275bb718e/compiler/ras/ILValidationRules.cpp#L436-L470, seems like it is checking the children of the regStore node and fails when child is PassThrough which does not have a type.

EDIT: I can probably make change to this validator to expect such kind of node under pass through. Just wanted to confirm that is the right way and we can make an exception for PassThrough under regstore.

andrewcraik · 2020-09-02T13:42:38Z

@r30shah I think the validator needs to be updated to handle the specific case it is tripping on - I think we need to be careful not to allow PassThroughs too generally so I think you need to make the exception fairly specific. FYI @Leonardo2718 given your interest in the validator.

r30shah · 2020-09-02T13:49:02Z

@andrewcraik Change I was thinking of was to get the real child (Iterate through the child of PassThrough till child is not passthrough) and get validator check the real child. I agree, we should not allow PassThrough too generally

Leonardo2718 · 2020-09-02T14:38:29Z

I agree that adding a special case to the validator for a PassThrough under a RegStore is the reasonable thing to do right now. I also like @r30shah's suggestion of checking the type of the "real" (non-passthrough) child in those special cases. To avoid cluttering the validation code, I suggest implementing that check in a separate function and modifying TR::ValidateChildTypes::validate() to call that function, something like (subject to cleanup):

          if (childOpcode.getOpCodeValue() != TR::GlRegDeps) 
             { 
             const auto expChildType = opcode.expectedChildType(i); 
             const auto expChildTypeName = (expChildType >= TR::NumTypes) ? 
                                            "UnspecifiedChildType" : 
                                            TR::DataType::getName(expChildType); 
             if (opcode.isRegStore() && childOpcode.getOpCodeValue() == TR::PassThrough)
                {
                checkPassThroughChildRecursively(expChildType, expChildTypeName, node->getChild(i))
                }
             else
                {
                const auto actChildType = childOpcode.getDataType().getDataType();
                const auto actChildTypeName = TR::DataType::getName(actChildType); 
                TR::checkILCondition(node, (expChildType >= TR::NumTypes || actChildType == expChildType), 
                                  comp(), "Child %d has unexpected type %s (expected %s)", 
                                  i, actChildTypeName, expChildTypeName); 
                }
             }

Also, just as an FYI, #5250 still needs to be undone/reverted in order to re-enable regDepCopyRemoval when value types are enabled.

r30shah · 2020-09-02T14:50:05Z

compiler/ras/ILValidationRules.cpp

@@ -446,6 +446,20 @@ void TR::ValidateChildTypes::validate(TR::Node *node)
         auto childOpcode = node->getChild(i)->getOpCode();
         if (childOpcode.getOpCodeValue() != TR::GlRegDeps)
            {
+				/**


@Leonardo2718 Can I get review on this change? I just saw the code you recommended, but before that did try following changes and test is passing with it.

Isn't this effectively allowing a PassThrough to appear anywhere, as long as the final child is the correct type? My understanding was that it should only be allowed in this one specific case (i.e. under a regStore).

The structure of the code itself looks fine, although I would suggest moving the code bellow the const variable definitions so you can just use those instead of having to call childOpcode.getDataType().getDataType(), etc.

My understanding for the PassThrough is as its name suggests, it is just a pass through node and it is OK to have passthrough child as long as real child under passthrough matches the expected child type. Although I can not find anywhere in OMR and OpenJ9 where it uses it like this.

In https://github.com/eclipse/omr/compare/c20ca06dcf272a1aa1fae1082612c7f5bb8b28c4..e2bb845af7cf4df7240431d864af758e3afee04c Made the changes to keep it limited to PassThrough under regStore only,.

In general the compiler will minimize PassThroughs - they are more a transient effect of transformation - if they are allowed to proliferate then we can have a problem because the pattern matching in the compiler will begin to fail. Allowing this only under a regStore makes sense.

RegDepCopyRemoval is the last ran optimization which intends to reduce register shuffling due to global register dependencies. It was generating tree where we have a PassThrough Child under PassThrough to copy node to new virtual register. This tree was not working well with the post GRA block splitter. Issue was that PassThrough child encountered in GlRegDeps was anchored to tree top. In postGRA block splitter, it records the regStores encountered before the GlRegDeps in extended basic block to guide decision on which register to chose for the node after split point.In this commit, adding changes to both splitPostGRA block splitter and RegDepCopyRemoval optimization so that we can split the blocks after later has run. Signed-off-by: Rahil Shah <[email protected]>

It is possible to have a PassThrough as a child node of regular node. When we are validating the children of a node, we need to make sure that when we see a PassThrough child, it gets real child that represents the node and check its type with parent. Signed-off-by: Rahil Shah <[email protected]>

Leonardo2718

LGTM 👍

andrewcraik · 2020-09-08T14:59:43Z

@genie-omr build all

r30shah · 2020-09-11T13:51:36Z

All the tests have been passed. @andrewcraik If you are OK with the changes, we can merge this.

r30shah requested review from andrewcraik, Leonardo2718 and vijaysun-omr as code owners May 20, 2020 16:44

r30shah changed the title ~~Changes in RegDepCopyRemoval for splitPostGRA block splitter~~ WIP: Changes in RegDepCopyRemoval for splitPostGRA block splitter May 20, 2020

andrewcraik added the comp:compiler label May 20, 2020

andrewcraik self-assigned this May 20, 2020

r30shah force-pushed the regDepCopyRemovalChanges branch from c967f9f to efa37c8 Compare May 26, 2020 20:23

r30shah mentioned this pull request May 27, 2020

Investigate and Fix issues with RegDepCopyRemoval Optimization with SplitPostGRA eclipse-openj9/openj9#9712

Open

r30shah force-pushed the regDepCopyRemovalChanges branch from efa37c8 to e9b81e5 Compare May 29, 2020 20:30

r30shah force-pushed the regDepCopyRemovalChanges branch from e9b81e5 to 1a8205d Compare August 31, 2020 21:38

r30shah changed the title ~~WIP: Changes in RegDepCopyRemoval for splitPostGRA block splitter~~ Changes in RegDepCopyRemoval for splitPostGRA block splitter Aug 31, 2020

andrewcraik reviewed Sep 1, 2020

View reviewed changes

r30shah requested review from 0xdaryl and mstoodle as code owners September 2, 2020 14:41

r30shah commented Sep 2, 2020

View reviewed changes

r30shah force-pushed the regDepCopyRemovalChanges branch from 6c17a6b to 51075a6 Compare September 2, 2020 14:54

r30shah force-pushed the regDepCopyRemovalChanges branch from 51075a6 to c20ca06 Compare September 2, 2020 15:18

r30shah force-pushed the regDepCopyRemovalChanges branch from c20ca06 to e2bb845 Compare September 2, 2020 15:51

Leonardo2718 approved these changes Sep 2, 2020

View reviewed changes

andrewcraik approved these changes Sep 8, 2020

View reviewed changes

andrewcraik merged commit 637ebe0 into eclipse-omr:master Sep 11, 2020

r30shah mentioned this pull request Sep 17, 2020

JDK11 Linux ppc64le java/util/Hashtable/SerializationDeadlock.java *** stack smashing detected *** eclipse-openj9/openj9#10630

Closed

Leonardo2718 mentioned this pull request Sep 23, 2020

JIT assert RegDepCopyRemoval.cpp:441 VMState: 0x00055aff eclipse-openj9/openj9#10671

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes in RegDepCopyRemoval for splitPostGRA block splitter #5219

Changes in RegDepCopyRemoval for splitPostGRA block splitter #5219

r30shah commented May 20, 2020

r30shah commented May 20, 2020 •

edited

Loading

andrewcraik commented May 20, 2020

andrewcraik commented Aug 27, 2020

r30shah commented Aug 27, 2020

r30shah commented Sep 1, 2020

andrewcraik Sep 1, 2020

r30shah Sep 2, 2020

andrewcraik commented Sep 1, 2020

r30shah commented Sep 1, 2020 •

edited

Loading

andrewcraik commented Sep 2, 2020

r30shah commented Sep 2, 2020

Leonardo2718 commented Sep 2, 2020

r30shah Sep 2, 2020

Leonardo2718 Sep 2, 2020

r30shah Sep 2, 2020 •

edited

Loading

andrewcraik Sep 8, 2020

Leonardo2718 left a comment

andrewcraik commented Sep 8, 2020

r30shah commented Sep 11, 2020

Changes in RegDepCopyRemoval for splitPostGRA block splitter #5219

Changes in RegDepCopyRemoval for splitPostGRA block splitter #5219

Conversation

r30shah commented May 20, 2020

r30shah commented May 20, 2020 • edited Loading

andrewcraik commented May 20, 2020

andrewcraik commented Aug 27, 2020

r30shah commented Aug 27, 2020

r30shah commented Sep 1, 2020

andrewcraik Sep 1, 2020

Choose a reason for hiding this comment

r30shah Sep 2, 2020

Choose a reason for hiding this comment

andrewcraik commented Sep 1, 2020

r30shah commented Sep 1, 2020 • edited Loading

andrewcraik commented Sep 2, 2020

r30shah commented Sep 2, 2020

Leonardo2718 commented Sep 2, 2020

r30shah Sep 2, 2020

Choose a reason for hiding this comment

Leonardo2718 Sep 2, 2020

Choose a reason for hiding this comment

r30shah Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

andrewcraik Sep 8, 2020

Choose a reason for hiding this comment

Leonardo2718 left a comment

Choose a reason for hiding this comment

andrewcraik commented Sep 8, 2020

r30shah commented Sep 11, 2020

r30shah commented May 20, 2020 •

edited

Loading

r30shah commented Sep 1, 2020 •

edited

Loading

r30shah Sep 2, 2020 •

edited

Loading