Reset _defMergedNodes in global VP on every basic block

[gita-omr]

defMergedNodes is supposed to keep track of commonned nodes.
So it's safe to reset it on every basic block. Also add an assert
that there are no extended blocks during global VP.

[gita-omr]

The fix is based on @vijaysun-omr suggestion. @jdmpapin maybe you can review?

[gita-omr]

Related:
[1] de38489

[jdmpapin]

The change looks safe. I think this will help the case where an outermost loop has no other nested regions within it. Are there other cases it's meant to benefit? If it's only that one case, it might be possible to express the intention more clearly by adding a call to _defMergedNodes->empty() right after the first pass, e.g.

diff --git a/compiler/optimizer/OMRValuePropagation.cpp b/compiler/optimizer/OMRValuePropagation.cpp
index b932647da..7720eb87b 100644
--- a/compiler/optimizer/OMRValuePropagation.cpp
+++ b/compiler/optimizer/OMRValuePropagation.cpp
@@ -4142,6 +4142,14 @@ void TR::GlobalValuePropagation::processNaturalLoop(TR_StructureSubGraphNode *no
       _visitCount--;
       processRegionSubgraph(node, false, true, true);

+      // Reset the nodes seen in mergeDefConstraints() between passes through
+      // the loop. This is needed to recognize that loads within the loop have
+      // not yet been evaluated the second time through in case there are no
+      // nested regions (or possibly in case there is a block directly in the
+      // loop which we process after all nested regions the first time, but
+      // somehow before all nested regions the second time).
+      _defMergedNodes->empty();
+
       // having processed the loop the first time we want to make sure to wipe out any
       // seenOnAllPaths information on the back edges - we use this notion only for the current iteration
       for (auto itr = region->getEntry()->getPredecessors().begin(), end = region->getEntry()->getPredecessors().end(); itr != end; ++itr)

[jdmpapin]

LGTM - my other comment is only a suggestion

[gita-omr]

I think the idea is that _defMergedNodes essentially is just keeping track of commoned nodes and therefore could be reset on every extended block. So resetting it in processRegionSubgraph would be the closest to doing that.

@jdmpapin @vijaysun-omr

[jdmpapin]

OK, I buy that logic 🙂

By the way, I got worried about extended blocks as I was reading this code yesterday. If GVP can encounter extended blocks, then I think the use of _defMergedNodes is already broken in one or maybe two ways, unrelated to your changes. But from looking at all of the strategies, I believe GVP doesn't run after block extension. (It looks like it's technically possible that it could in OpenJ9, but only with -Xjit:enableSequentialLoadStoreCold)

[jdmpapin]

Come to think of it, maybe we should just assert that no block is an extension of another and reset it every block

[gita-omr]

Thanks @jdmpapin ! I just a bit worry that we might want to run GVP on extended blocks in the future, but I guess you are saying it's broken already... Not sure if we should fix that or assert. I would let @vijaysun-omr comment...

[vijaysun-omr]

I think this may get a bit more complicated if we ran GVP on extended basic blocks (which I think we don't in any strategy currently). For example, we may run into a case when we start an extended basic block inside a loop but take the loop back edge midway through that extended basic block and the remainder of the extended basic block is outside the loop. Since GVP does its processing for a natural loop at a time, this could complicate matters about what we want to do with the bit vector for visited status when we are analyzing the part of the extended basic block that is outside the loop.

I'd prefer not to have to deal with this right now and add an assertion instead if we detected extended basic blocks in GVP (giving this above rationale for the assert) if you agree, especially as I did'nt think we allowed GVP for extended blocks presently.

This may have been what you were thinking too (@jdmpapin and @gita-omr) in which case we should go ahead.

[jdmpapin]

Thanks for the great example, @vijaysun-omr. I'll elaborate on the issues that I was thinking of too, just for additional motivation

First consider a diamond within an acyclic region A→B, A→C, B→D, C→D:

  A
 / \
B   C
 \ /
  D

Suppose that A, B, are blocks and C is a nested region, e.g. a loop, and suppose further that B is an extension of A. I don't believe there's anything to stop GVP from processing these nodes in the order A, C, B, D, but if we process them in that order, we'll inappropriately reset _defMergedNodes between A and B.

For the second example - I'm less sure about this one, but - I think it might be possible for an acyclic region or improper region to start partway through an extended block, i.e. so that the entry block of the region is an extension of a block outside. If so, then when we reset _defMergedNodes at the beginning of the region, that will be partway through an extended block

[vijaysun-omr]

I think those are both good examples, Devin and your comment as a whole may be worth adding to the comment next to the assert if that we what we are planning to add to GVP.

[jdmpapin]

Would you mind using TR_ASSERT_FATAL here? I think that TR_ASSERT still doesn't get checked in most (or even all?) of the testing

[gita-omr]

Yeah, I was wondering about that but all asserts in this file are non-fatal. Anyways, let me make it FATAL just to be sure.

[vijaysun-omr]

Jenkins build all

[gita-omr]

Made WIP while I am retesting with TR_ASSERT_FATAL. Not sure why but I somehow believed that all asserts are fatal now...

[jdmpapin]

Since we seem to be going with the reset on every block with an assertion, would you mind squashing the two commits, @gita-omr? The first commit would just be noise in the history

[gita-omr]

Since we seem to be going with the reset on every block with an assertion, would you mind squashing the two commits, @gita-omr? The first commit would just be noise in the history

Definitely, but let me finish all the testing first. BTW: please note the change in getting the block out of the node. Note sure how the first commit worked...

[gita-omr]

All tests passed. Squashed the commits.

@jdmpapin @vijaysun-omr

[vijaysun-omr]

Jenkins build all

[rmnattas]

Merging this also solves GHE openj9-jit-power#207

[vijaysun-omr]

Checks have passed and reviews are also done.