[NewGVN] Improve phi-of-ops fix to allow loads that loop invariant-ish

[nlopes]

Summary

My previous fix for the phi-of-ops was a bit of a big hammer.
This patch uses Memory SSA to detect when a load is guaranteed to not depend on a store that may happen inside the loop.

This patch fixes the regression in storeoverstore.ll, where the big hammer had disabled phi-of-ops for this test case.

[asbirlea]

Thank you for looking at this! Please see inline.

llvm/lib/Transforms/Scalar/NewGVN.cpp

2601

if (!OriginalAccess)
  return true;

(if instruction is not modeled in MSSA)

2612

A clobbering access (DefiningAccess) always dominates the queried access (OriginalAccess), so the second condition is redundant.
Now, is the first condition (different BBs) enough? I don't think it is. The store can still be inside the loop but in an earlier block.

BBa (store)  -> BBb -> BBd (load) -
    ^        \-> BBc /^          |
    |____________________________|

Inserting the phi-of-ops in BBa is incorrect, if the load will use the value from phi-of-ops instead of reloading. This example is what I was seeing in one of the bug reports (one load was in BBa and it's covered by the tests here, another was in BBd.

I think the condition here is something like if (DT->properlyDominates(DefiningAccess->getBlock(), PHIBlock)).

[nlopes]

Subsumed by https://reviews.llvm.org/D156055

llvm/lib/Transforms/Scalar/NewGVN.cpp

bool NewGVN::OpIsSafeForPHIOfOpsHelper(
   // PHI in the same block.
   if (isa<PHINode>(V) && getBlockForValue(V) == PHIBlock) {
     OpSafeForPHIOfOps.insert({V, false});
     return false;
   }
 
   auto *OrigI = cast<Instruction>(V);
   // When we hit an instruction that reads memory (load, call, etc), we must
-  // consider any store that may happen in the loop. For now, we assume the
-  // worst: there is a store in the loop that alias with this read.
+  // consider any store that may happen in the loop.
   // The case where the load is outside the loop is already covered by the
   // dominator check above.
-  // TODO: relax this condition
-  if (OrigI->mayReadFromMemory())
+  if (OrigI->mayReadFromMemory()) {
+    MemoryAccess *OriginalAccess = getMemoryAccess(OrigI);
+    MemoryAccess *DefiningAccess =
+        MSSAWalker->getClobberingMemoryAccess(OriginalAccess);
+
+    // A loop invariant load.
+    if (MSSA->isLiveOnEntryDef(DefiningAccess))
+      return true;
+
+    // A store must happen outside the loop in a dominator as otherwise we
+    // could pick the previous value in the phi-of-ops.
+    if (OriginalAccess->getBlock() != DefiningAccess->getBlock() &&
+        MSSA->dominates(DefiningAccess, OriginalAccess))
+      return true;
+
     return false;
+  }
 
   for (auto *Op : OrigI->operand_values()) {
     if (!isa<Instruction>(Op))
       continue;
     // Stop now if we find an unsafe operand.
     auto OISIt = OpSafeForPHIOfOps.find(OrigI);
     if (OISIt != OpSafeForPHIOfOps.end()) {
       if (!OISIt->second) {

llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll

bb229:
   %inc = add i8 %idx, 1
   store i8 %inc, i8* %q
   br label %bb57
 
 bb237:
   ret void
 }
 
-; TODO: we should support this case
-define void @no-alias-store-in-loop(i8* noalias %p, i8* noalias %q) {
-; CHECK-LABEL: @no-alias-store-in-loop(
+define void @store-in-loop2(i8* %p, i8* %q) {
+; CHECK-LABEL: @store-in-loop2(
 ; CHECK-NEXT:  bb56:
 ; CHECK-NEXT:    br label [[BB57:%.*]]
 ; CHECK:       bb57:
 ; CHECK-NEXT:    [[N59:%.*]] = phi i1 [ false, [[BB229:%.*]] ], [ true, [[BB56:%.*]] ]
 ; CHECK-NEXT:    [[IDX:%.*]] = phi i8 [ 0, [[BB56]] ], [ [[INC:%.*]], [[BB229]] ]
+; CHECK-NEXT:    store i8 [[IDX]], i8* [[Q:%.*]], align 1
 ; CHECK-NEXT:    [[N60:%.*]] = load i8, i8* [[P:%.*]], align 1
 ; CHECK-NEXT:    [[N62:%.*]] = icmp ne i8 [[N60]], 2
 ; CHECK-NEXT:    [[N63:%.*]] = or i1 [[N59]], [[N62]]
 ; CHECK-NEXT:    br i1 [[N63]], label [[BB229]], label [[BB237:%.*]]
 ; CHECK:       bb229:
 ; CHECK-NEXT:    [[INC]] = add i8 [[IDX]], 1
+; CHECK-NEXT:    br label [[BB57]]
+; CHECK:       bb237:
+; CHECK-NEXT:    ret void
+;
+bb56:
+  br label %bb57
+
+bb57:
+  %n59 = phi i1 [ false, %bb229 ], [ true, %bb56 ]
+  %idx = phi i8 [0, %bb56], [%inc, %bb229]
+  store i8 %idx, i8* %q
+  %n60 = load i8, i8* %p
+  %n62 = icmp ne i8 %n60, 2
+  %n63 = or i1 %n59, %n62
+  br i1 %n63, label %bb229, label %bb237
+
+bb229:
+  %inc = add i8 %idx, 1
+  br label %bb57
+
+bb237:
+  ret void
+}
+
+define void @hoisted-store(i8* %p, i8* %q) {
+; CHECK-LABEL: @hoisted-store(
+; CHECK-NEXT:  bb56:
+; CHECK-NEXT:    store i8 3, i8* [[Q:%.*]], align 1
+; CHECK-NEXT:    br label [[BB57:%.*]]
+; CHECK:       bb57:
+; CHECK-NEXT:    [[PHIOFOPS:%.*]] = phi i1 [ true, [[BB56:%.*]] ], [ [[N62:%.*]], [[BB229:%.*]] ]
+; CHECK-NEXT:    [[N59:%.*]] = phi i1 [ false, [[BB229]] ], [ true, [[BB56]] ]
+; CHECK-NEXT:    [[IDX:%.*]] = phi i8 [ 0, [[BB56]] ], [ [[INC:%.*]], [[BB229]] ]
+; CHECK-NEXT:    [[N60:%.*]] = load i8, i8* [[P:%.*]], align 1
+; CHECK-NEXT:    [[N62]] = icmp ne i8 [[N60]], 2
+; CHECK-NEXT:    br i1 [[PHIOFOPS]], label [[BB229]], label [[BB237:%.*]]
+; CHECK:       bb229:
+; CHECK-NEXT:    [[INC]] = add i8 [[IDX]], 1
+; CHECK-NEXT:    br label [[BB57]]
+; CHECK:       bb237:
+; CHECK-NEXT:    ret void
+;
+bb56:
+  store i8 3, i8* %q
+  br label %bb57
+
+bb57:
+  %n59 = phi i1 [ false, %bb229 ], [ true, %bb56 ]
+  %idx = phi i8 [0, %bb56], [%inc, %bb229]
+  %n60 = load i8, i8* %p
+  %n62 = icmp ne i8 %n60, 2
+  %n63 = or i1 %n59, %n62
+  br i1 %n63, label %bb229, label %bb237
+
+bb229:
+  %inc = add i8 %idx, 1
+  br label %bb57
+
+bb237:
+  ret void
+}
+
+define void @no-alias-store-in-loop(i8* noalias %p, i8* noalias %q) {
+; CHECK-LABEL: @no-alias-store-in-loop(
+; CHECK-NEXT:  bb56:
+; CHECK-NEXT:    br label [[BB57:%.*]]
+; CHECK:       bb57:
+; CHECK-NEXT:    [[PHIOFOPS:%.*]] = phi i1 [ true, [[BB56:%.*]] ], [ [[N62:%.*]], [[BB229:%.*]] ]
+; CHECK-NEXT:    [[N59:%.*]] = phi i1 [ false, [[BB229]] ], [ true, [[BB56]] ]
+; CHECK-NEXT:    [[IDX:%.*]] = phi i8 [ 0, [[BB56]] ], [ [[INC:%.*]], [[BB229]] ]
+; CHECK-NEXT:    [[N60:%.*]] = load i8, i8* [[P:%.*]], align 1
+; CHECK-NEXT:    [[N62]] = icmp ne i8 [[N60]], 2
+; CHECK-NEXT:    br i1 [[PHIOFOPS]], label [[BB229]], label [[BB237:%.*]]
+; CHECK:       bb229:
+; CHECK-NEXT:    [[INC]] = add i8 [[IDX]], 1
 ; CHECK-NEXT:    store i8 [[INC]], i8* [[Q:%.*]], align 1
 ; CHECK-NEXT:    br label [[BB57]]
 ; CHECK:       bb237:
 ; CHECK-NEXT:    ret void
 ;
 bb56:
   br label %bb57
 

 bb229:
   call void @f()
   br label %bb57
 
 bb237:
   ret void
 }
 
-; TODO: we should support this case
 define void @nowrite-function-in-loop(i8* %p) {
 ; CHECK-LABEL: @nowrite-function-in-loop(
 ; CHECK-NEXT:  bb56:
 ; CHECK-NEXT:    br label [[BB57:%.*]]
 ; CHECK:       bb57:
-; CHECK-NEXT:    [[N59:%.*]] = phi i1 [ false, [[BB229:%.*]] ], [ true, [[BB56:%.*]] ]
+; CHECK-NEXT:    [[PHIOFOPS:%.*]] = phi i1 [ true, [[BB56:%.*]] ], [ [[N62:%.*]], [[BB229:%.*]] ]
+; CHECK-NEXT:    [[N59:%.*]] = phi i1 [ false, [[BB229]] ], [ true, [[BB56]] ]
 ; CHECK-NEXT:    [[N60:%.*]] = load i8, i8* [[P:%.*]], align 1
-; CHECK-NEXT:    [[N62:%.*]] = icmp ne i8 [[N60]], 2
-; CHECK-NEXT:    [[N63:%.*]] = or i1 [[N59]], [[N62]]
-; CHECK-NEXT:    br i1 [[N63]], label [[BB229]], label [[BB237:%.*]]
+; CHECK-NEXT:    [[N62]] = icmp ne i8 [[N60]], 2
+; CHECK-NEXT:    br i1 [[PHIOFOPS]], label [[BB229]], label [[BB237:%.*]]
 ; CHECK:       bb229:
 ; CHECK-NEXT:    call void @f() #[[ATTR0:[0-9]+]]
 ; CHECK-NEXT:    br label [[BB57]]
 ; CHECK:       bb237:
 ; CHECK-NEXT:    ret void
 ;
 bb56:
   br label %bb57

llvm/test/Transforms/NewGVN/storeoverstore.ll

 define i32 @foo(i32*, i32)  {
 ; CHECK-LABEL: @foo(
 ; CHECK-NEXT:    store i32 5, i32* [[TMP0:%.*]], align 4
 ; CHECK-NEXT:    [[TMP3:%.*]] = icmp ne i32 [[TMP1:%.*]], 0
 ; CHECK-NEXT:    br i1 [[TMP3]], label [[TMP4:%.*]], label [[TMP5:%.*]]
 ; CHECK:       4:
 ; CHECK-NEXT:    br label [[TMP5]]
 ; CHECK:       5:
-; CHECK-NEXT:    [[DOT0:%.*]] = phi i32 [ 10, [[TMP4]] ], [ 5, [[TMP2:%.*]] ]
-; CHECK-NEXT:    br i1 [[TMP3]], label [[TMP6:%.*]], label [[TMP8:%.*]]
+; CHECK-NEXT:    [[PHIOFOPS:%.*]] = phi i32 [ 10, [[TMP2:%.*]] ], [ 15, [[TMP4]] ]
+; CHECK-NEXT:    [[DOT0:%.*]] = phi i32 [ 10, [[TMP4]] ], [ 5, [[TMP2]] ]
+; CHECK-NEXT:    br i1 [[TMP3]], label [[TMP6:%.*]], label [[TMP7:%.*]]
 ; CHECK:       6:
-; CHECK-NEXT:    [[TMP7:%.*]] = add nsw i32 [[DOT0]], 5
-; CHECK-NEXT:    br label [[TMP8]]
-; CHECK:       8:
-; CHECK-NEXT:    [[DOT1:%.*]] = phi i32 [ [[TMP7]], [[TMP6]] ], [ [[DOT0]], [[TMP5]] ]
+; CHECK-NEXT:    br label [[TMP7]]
+; CHECK:       7:
+; CHECK-NEXT:    [[DOT1:%.*]] = phi i32 [ [[PHIOFOPS]], [[TMP6]] ], [ [[DOT0]], [[TMP5]] ]
 ; CHECK-NEXT:    ret i32 [[DOT1]]
 ;
   store i32 5, i32* %0, align 4
   %3 = icmp ne i32 %1, 0
   br i1 %3, label %4, label %7
 
 ; <label>:4:                                      ; preds = %2
   %5 = load i32, i32* %0, align 4

 ; CHECK-NEXT:    store i32 5, i32* [[TMP0:%.*]], align 4
 ; CHECK-NEXT:    [[TMP3:%.*]] = icmp ne i32 [[TMP1:%.*]], 0
 ; CHECK-NEXT:    br i1 [[TMP3]], label [[TMP4:%.*]], label [[TMP5:%.*]]
 ; CHECK:       4:
 ; CHECK-NEXT:    br label [[TMP6:%.*]]
 ; CHECK:       5:
 ; CHECK-NEXT:    br label [[TMP6]]
 ; CHECK:       6:
+; CHECK-NEXT:    [[PHIOFOPS:%.*]] = phi i32 [ 10, [[TMP5]] ], [ 15, [[TMP4]] ]
 ; CHECK-NEXT:    [[DOT0:%.*]] = phi i32 [ 10, [[TMP4]] ], [ 5, [[TMP5]] ]
-; CHECK-NEXT:    br i1 [[TMP3]], label [[TMP7:%.*]], label [[TMP9:%.*]]
+; CHECK-NEXT:    br i1 [[TMP3]], label [[TMP7:%.*]], label [[TMP8:%.*]]
 ; CHECK:       7:
-; CHECK-NEXT:    [[TMP8:%.*]] = add nsw i32 [[DOT0]], 5
-; CHECK-NEXT:    br label [[TMP9]]
-; CHECK:       9:
-; CHECK-NEXT:    [[DOT1:%.*]] = phi i32 [ [[TMP8]], [[TMP7]] ], [ [[DOT0]], [[TMP6]] ]
+; CHECK-NEXT:    br label [[TMP8]]
+; CHECK:       8:
+; CHECK-NEXT:    [[DOT1:%.*]] = phi i32 [ [[PHIOFOPS]], [[TMP7]] ], [ [[DOT0]], [[TMP6]] ]
 ; CHECK-NEXT:    ret i32 [[DOT1]]
 ;
   store i32 5, i32* %0, align 4
   %3 = icmp ne i32 %1, 0
   br i1 %3, label %4, label %7
 
 ; <label>:4:                                      ; preds = %2
   %5 = load i32, i32* %0, align 4