[PATCH] D16100: Fix for two constant propagation problems in GVN with assume intrinsic instruction

[Yuanrui Zhang via llvm-commits]

Ray added inline comments.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2137
@@ +2136,3 @@
+/// If DominatesByEdge is false, it means that there might be multiple edges
+/// from Root.Start to Root.End, in which case we need to pass Root.Start and
+/// use a different implementation of replaceDominatedUsesWith to test.
----------------
dberlin wrote:
> If there are multiple edges to the same BB, we should not do the replacement at all (we lack post-dominance info necessary to be safe in that case).
Note that, there is a condition about "multiple edges to the same BB":  it is from Root.Start to Root.End. That's when DominatesByEdge is set to false, and that's exactly the reason why DominatesByEdge was introduced as a parameter to this function.  See the example added before

@_Z1ii  in test/Transforms/GVN/assume-equal.ll 

; This test checks if constant propagation works for multiple node edges
; CHECK-LABEL: define i32 @_Z1ii(i32 %p)
define i32 @_Z1ii(i32 %p) {
entry:
  %cmp = icmp eq i32 %p, 42
  call void @llvm.assume(i1 %cmp)
  
  ; CHECK: br i1 true, label %bb2, label %bb2
  br i1 %cmp, label %bb2, label %bb2
bb2:
  ; CHECK: br i1 true, label %bb2, label %bb2
  br i1 %cmp, label %bb2, label %bb2
  
  ; CHECK: ret i32 42
  ret i32 %p
}

I was trying to make the comments in more detail. But if it is confusing, we can improve it.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2201
@@ -2197,3 +2200,3 @@
               ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)
-              : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getEnd());
+              : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getStart());
 
----------------
dberlin wrote:
> If this problem only occurs with assume instructions, we shouldn't change the generic code.
> If it's not a problem that only occur with assume instructions, we need
> A. more testcases.
> B. a more complete description of the problem that is happening and why this is the correct fix.
> 
===This is a general problem in GVN::propagateEquality with the following code:  

unsigned NumReplacements =
          DominatesByEdge
              ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)
              : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getStart());

When DominatesByEdge is TRUE, it passes the edge "Root" to replaceDominatedUsesWith, and we are fine. Since, if the edge "Root" dominates a USE, then Root.Start must dominates the USE.

When DominatesByEdge is FALSE, it passes the Root.End to replaceDominatedUsesWith, and we have a problem here.  Since, if Root.End dominates a USE, it does not mean that Root.Start dominates the USE. We show in the test case _Z1im below. 

In this case, we ideally need "RootDominatesEnd" (in GVN::propagateEquality) to further test before calling replaceDominatedWith(...Root.End), e.g.:

unsigned NumReplacements =
          DominatesByEdge
              ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)
              : 
          RootDominatesEnd? replaceDominatedUsesWith(LHS, RHS, *DT, Root.getEnd()) : 0;

However, when we cannot use the current RootDominatesEnd defined as below to test:

  // For speed, compute a conservative fast approximation to
  // DT->dominates(Root, Root.getEnd());
  bool RootDominatesEnd = isOnlyReachableViaThisEdge(Root, DT);

The reason is that, it only works when DominatesByEdge=TRUE, not when DominatesByEdge =FALSE where there are multiple edges (e.g., 2 edges) from Root.Start to Root.End, see @_Z1ii  in test/Transforms/GVN/assume-equal.ll again.

And, we cannot use the following either, 

boot RootDominatesEnd = DT->dominates(Root, Root.getEnd());

since, it ran into the problem Prazek saw before, giving an assert when DominatesByEdge=FALSE.  see @_Z1ii  in test/Transforms/GVN/assume-equal.ll again

Now, If we modify RootDominatesEnd to 

bool RootDominatesEnd = DT.dominates(Root.Start(), Root.End()), then it can be used:

unsigned NumReplacements =
          DominatesByEdge
              ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)
              : 
          RootDominatesEnd? replaceDominatedUsesWith(LHS, RHS, *DT, Root.getEnd()) : 0;

I didn't post this change, because 1) DT.dominates will slow down as comments show 2) the name RootDominatesEnd is not proper.

In fact, the simplest way is to 
1) pass Root.Start  to replaceDominatedUsesWith,  
2) then change replaceDominatedUsesWith(... BasicBlock* BB)  to replace the uses by the "END" of the BB. 
These two changes together will give exactly the same effect as when passing an edge, i.e. ,replaceDominatedUsesWith(.....   BasicBlockEdge Root).

Note that, currently GVN is the only caller of replaceDominatedUsesWith( ...,    BasicBlock* BB).  So, it is safe to change to use "properlyDominates".

===Although it is a general problem with propagateEquality, I don't have any test cases other than those with assume instructions. The reason is that, 

1) The problem only exists when DominatedByEdge = FALSE
2) GVN::processAssumeIntrinsic is the only caller to propagateEquality when DominatedByEdge=FALSE as below:

  for (BasicBlock *Successor : successors(IntrinsicI->getParent())) {
   
     BasicBlockEdge Edge(IntrinsicI->getParent(), Successor);
     
     Changed |= propagateEquality(V, True, Edge, false);
  
   }

================
Comment at: lib/Transforms/Utils/Local.cpp:1540
@@ -1539,3 +1539,3 @@
     auto *I = cast<Instruction>(U.getUser());
-    if (DT.dominates(BB, I->getParent())) {
+    if (DT.properlyDominates(BB, I->getParent())) {
       U.set(To);
----------------
dberlin wrote:
> This is definitely not correct.
> Local to a block, all uses are dominated by their definition, so you don't need properlyDominates to replace them legally.
> It's up to the caller to determine whether this is safe *semantically*.
> 
> (Even if this change was correct, you've also now made the replacement functions completely inconsistent with each other.)
Since propagateEquality in GVN is the only caller, that's why we changed the comments to inform other users that, this function will replace USES from the END of the BB.

If the called wants to replace the uses in the BB itself, the other overloaded interface replaceDomiantedUsesWith(..... BasicBlockEdge Root) can be used.


http://reviews.llvm.org/D16100