[GVN] Improve analysis for missed optimization remark

[hnrklssn]

Summary

This change tries to handle multiple dominating users of the pointer operand
by choosing the most immediately dominating one, if possible.
While making this change I also found that the previous implementation
had a missing break statement, making all loads with an odd number of
dominating users emit an OtherAccess value, so that has also been fixed.

[anemet]

Thanks for improving this. Can you please add a small test highlighting where your change improve the diagnostics? TIA!

[hnrklssn]

[GVN] Update analysis for optimization remark and add tests

We now cover even more cases for finding OtherAccess.

[hnrklssn]

This is my first time using both Phabricator and Arcanist, so I'm not entirely familiar with the UI and workflow, but I can't seem to find one of the test cases I added in my update. It's supposed to be in llvm/test/Transforms/GVN/opt-remarks-multiple-users.ll, but I can't see it added in the revision. Did I miss something? It's an earlier commit than the new diff I pushed, so I would've thought it would be included as well.

[hnrklssn]

[GVN] [Remarks] Trying to push missing file

[hnrklssn]

Updating D79097: [GVN] Improve analysis for missed optimization remark

Now all patches should be included... I think.

[hnrklssn]

Updating D79097: reformatting code according to pre-merge check

[anemet]

Sorry about the delay, just a few small nits on the tests, this is pretty close. Thanks for pushing this!

llvm/test/Transforms/GVN/opt-remarks-multiple-users.ll

4

Please add a comment with the intention for test. In this particular case I imagine that before your change we would have given up on determining OtherAccess.

45

I know this is a minimized testcase but don’t have unreachable here to make it more realistic.

llvm/test/Transforms/GVN/opt-remarks-non-dominating.ll

62

Please add a comment before each functions what it’s testing.

64

I would change these undefs to actual values to make these tests more realistic.

[anemet]

@hnrklssn hi, are you panning to get back to this at some point?

[hnrklssn]

In D79097#2218463, @anemet wrote:

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

[anemet]

In D79097#2218567, @hnrklssn wrote:

In D79097#2218463, @anemet wrote:

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

Oh cool, are you planning to publish/publicize this work once it's done?

[hnrklssn]

In D79097#2218587, @anemet wrote:

In D79097#2218567, @hnrklssn wrote:

In D79097#2218463, @anemet wrote:

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

Oh cool, are you planning to publish/publicize this work once it's done?

Yup, it will be published in LUP Student Papers afaict! I can send you a rough draft if you're interested. Most of the final content is in place already, but it's a bit rough around the edges.

[anemet]

In D79097#2218765, @hnrklssn wrote:

In D79097#2218587, @anemet wrote:

In D79097#2218567, @hnrklssn wrote:

In D79097#2218463, @anemet wrote:

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

Oh cool, are you planning to publish/publicize this work once it's done?

Yup, it will be published in LUP Student Papers afaict! I can send you a rough draft if you're interested. Most of the final content is in place already, but it's a bit rough around the edges.

No hurry, the final version is fine. You may also want to consider sending it to llvm-dev if you think it could be interesting to other people in the community.

[hnrklssn]

Addressed review comments and fixed a crash for non-local variables.

[hnrklssn]

After a long wait I'm now finally able to work on this again! Please excuse the delay.

llvm/test/Transforms/GVN/opt-remarks-non-dominating.ll

64

Missed this comment, will update again!

[hnrklssn]

Removed undefs in test cases.

[hnrklssn]

It should be ready to review again now :)

[anemet]

Glad, you were able to get back to this.

llvm/test/Transforms/GVN/opt-remarks-multiple-users.ll

15

It seems that we want to be able to distinguish between the two calls here?

If you add source debug info to test along the lines of GVN/opt-remarks.ll you should be able to check that the right call is returned here.

Also a nit, you may want to chop up the CHECK section so that each checking section is right before the function it corresponds too.

[hnrklssn]

Split CHECKs to keep them closer to their relevant functions, and added debug information to disambiguate instructions where relevant.

[hnrklssn]

Fixed formatting issue

[anemet]

Looks great, thanks!

[hnrklssn]

Cool :) This is my first submission, so I don't have commit access. If I understand correctly that means I need someone else to commit it on my behalf, yes?

[anemet]

Congrats ;). Yes, I will commit it for you. Can you please rebase and retest, looks like the patch is not applying cleanly.

[hnrklssn]

Rebasing to master

[hnrklssn]

Forgot to rebase onto upstream... That explains why it went so smoothly!

[hnrklssn]

Rebased onto upstream/main

[hnrklssn]

Ran clang-format

llvm/lib/Transforms/Scalar/GVN.cpp

 }
 
 static bool isLifetimeStart(const Instruction *Inst) {
   if (const IntrinsicInst* II = dyn_cast<IntrinsicInst>(Inst))
     return II->getIntrinsicID() == Intrinsic::lifetime_start;
   return false;
 }
 
+/// Assuming To can be reached from both From and Between, does Between lie on
+/// every path from From to To?
+static bool liesBetween(const Instruction *From, Instruction *Between,
+                        const Instruction *To, DominatorTree *DT) {
+  if (From->getParent() == Between->getParent())
+    return DT->dominates(From, Between);
+  SmallSet<BasicBlock *, 1> Exclusion;
+  Exclusion.insert(Between->getParent());
+  return !isPotentiallyReachable(From, To, &Exclusion, DT);
+}
+
 /// Try to locate the three instruction involved in a missed
 /// load-elimination case that is due to an intervening store.
 static void reportMayClobberedLoad(LoadInst *LI, MemDepResult DepInfo,
                                    DominatorTree *DT,
                                    OptimizationRemarkEmitter *ORE) {
   using namespace ore;
 
   User *OtherAccess = nullptr;
 
   OptimizationRemarkMissed R(DEBUG_TYPE, "LoadClobbered", LI);
   R << "load of type " << NV("Type", LI->getType()) << " not eliminated"
     << setExtraArgs();
 
-  for (auto *U : LI->getPointerOperand()->users())
-    if (U != LI && (isa<LoadInst>(U) || isa<StoreInst>(U)) &&
+  for (auto *U : LI->getPointerOperand()->users()) {
+    if (U != LI && (isa<LoadInst>(U) || isa<StoreInst>(U)) && cast<Instruction>(U)->getFunction() == LI->getFunction() &&
         DT->dominates(cast<Instruction>(U), LI)) {
-      // FIXME: for now give up if there are multiple memory accesses that
-      // dominate the load.  We need further analysis to decide which one is
-      // that we're forwarding from.
-      if (OtherAccess)
-        OtherAccess = nullptr;
+      // Use the most immediately dominating value
+      if (OtherAccess) {
+        if (DT->dominates(cast<Instruction>(OtherAccess), cast<Instruction>(U)))
+          OtherAccess = U;
         else
+          assert(DT->dominates(cast<Instruction>(U),
+                               cast<Instruction>(OtherAccess)));
+      } else
         OtherAccess = U;
     }
+  }
+
+  if (!OtherAccess) {
+    // There is no dominating use, check if we can find a closest non-dominating
+    // use that lies between any other potentially available use and LI.
+    for (auto *U : LI->getPointerOperand()->users()) {
+      if (U != LI && (isa<LoadInst>(U) || isa<StoreInst>(U)) &&
+          cast<Instruction>(U)->getFunction() == LI->getFunction() &&
+          isPotentiallyReachable(cast<Instruction>(U), LI, nullptr, DT)) {
+        if (OtherAccess) {
+          if (liesBetween(cast<Instruction>(OtherAccess), cast<Instruction>(U),
+                          LI, DT)) {
+            OtherAccess = U;
+          } else if (!liesBetween(cast<Instruction>(U),
+                                  cast<Instruction>(OtherAccess), LI, DT)) {
+            // These uses are both partially available at LI were it not for the
+            // clobber, but neither lies strictly after the other.
+            OtherAccess = nullptr;
+            break;
+          } // else: keep current OtherAccess since it lies between U and LI
+        } else {
+          OtherAccess = U;
+        }
+      }
+    }
+  }
 
   if (OtherAccess)
     R << " in favor of " << NV("OtherAccess", OtherAccess);
 
   R << " because it is clobbered by " << NV("ClobberedBy", DepInfo.getInst());
 
   ORE->emit(R);
 }

llvm/test/Transforms/GVN/opt-remarks-multiple-users.ll

+; RUN: opt < %s -gvn -o /dev/null  -pass-remarks-output=%t -S
+; RUN: cat %t | FileCheck %s
+
+; CHECK:      --- !Missed
+; CHECK-NEXT: Pass:            gvn
+; CHECK-NEXT: Name:            LoadClobbered
+; CHECK-NEXT: Function:        multipleUsers
+; CHECK-NEXT: Args:
+; CHECK-NEXT:   - String:          'load of type '
+; CHECK-NEXT:   - Type:            i32
+; CHECK-NEXT:   - String:          ' not eliminated'
+; CHECK-NEXT:   - String:          ' in favor of '
+; CHECK-NEXT:   - OtherAccess:     store
+; CHECK-NEXT:   - String:          ' because it is clobbered by '
+; CHECK-NEXT:   - ClobberedBy:     call
+; CHECK-NEXT: ...
+; CHECK:      --- !Missed
+; CHECK-NEXT: Pass:            gvn
+; CHECK-NEXT: Name:            LoadClobbered
+; CHECK-NEXT: Function:        multipleUsers
+; CHECK-NEXT: Args:
+; CHECK-NEXT:   - String:          'load of type '
+; CHECK-NEXT:   - Type:            i32
+; CHECK-NEXT:   - String:          ' not eliminated'
+; CHECK-NEXT:   - String:          ' in favor of '
+; CHECK-NEXT:   - OtherAccess:     load
+; CHECK-NEXT:   - String:          ' because it is clobbered by '
+; CHECK-NEXT:   - ClobberedBy:     call
+; CHECK-NEXT: ...
+; CHECK:      --- !Missed
+; CHECK-NEXT: Pass:            gvn
+; CHECK-NEXT: Name:            LoadClobbered
+; CHECK-NEXT: Function:        multipleUsers2
+; CHECK-NEXT: Args:
+; CHECK-NEXT:   - String:          'load of type '
+; CHECK-NEXT:   - Type:            i32
+; CHECK-NEXT:   - String:          ' not eliminated'
+; CHECK-NEXT:   - String:          ' in favor of '
+; CHECK-NEXT:   - OtherAccess:     store
+; CHECK-NEXT:   - String:          ' because it is clobbered by '
+; CHECK-NEXT:   - ClobberedBy:     call
+; CHECK-NEXT: ...
+; CHECK:      --- !Missed
+; CHECK-NEXT: Pass:            gvn
+; CHECK-NEXT: Name:            LoadClobbered
+; CHECK-NEXT: Function:        multipleUsers2
+; CHECK-NEXT: Args:
+; CHECK-NEXT:   - String:          'load of type '
+; CHECK-NEXT:   - Type:            i32
+; CHECK-NEXT:   - String:          ' not eliminated'
+; CHECK-NEXT:   - String:          ' in favor of '
+; CHECK-NEXT:   - OtherAccess:     load
+; CHECK-NEXT:   - String:          ' because it is clobbered by '
+; CHECK-NEXT:   - ClobberedBy:     call
+; CHECK-NEXT: ...
+
+; ModuleID = 'bugpoint-reduced-simplified.bc'
+source_filename = "gvn-test.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Tests that a last clobbering use can be determined even in the presence of
+; multiple users, given that one of them lies on a path between every other
+; potentially clobbering use and the load.
+
+define dso_local void @multipleUsers(i32* %a, i32 %b) local_unnamed_addr #0 {
+entry:
+  store i32 %b, i32* %a, align 4
+  tail call void @clobberingFunc() #1
+  %0 = load i32, i32* %a, align 4
+  tail call void @clobberingFunc() #1
+  %1 = load i32, i32* %a, align 4
+  %add2 = add nsw i32 %1, %0
+  ret void
+}
+
+; Ignore uses in other functions
+
+define dso_local void @multipleUsers2(i32 %b) local_unnamed_addr #0 {
+entry:
+  store i32 %b, i32* @g, align 4
+  tail call void @clobberingFunc() #1
+  %0 = load i32, i32* @g, align 4
+  tail call void @clobberingFunc() #1
+  %1 = load i32, i32* @g, align 4
+  %add3 = add nsw i32 %1, %0
+  ret void
+}
+
+declare dso_local void @clobberingFunc() local_unnamed_addr #0
+
+@g = external global i32
+
+define dso_local void @globalUser(i32 %b) local_unnamed_addr #0 {
+entry:
+  store i32 %b, i32* @g, align 4
+  ret void
+}
+
+
+attributes #0 = { "use-soft-float"="false" }
+attributes #1 = { nounwind }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 10.0.0 ([e-mail]:llvm/llvm-project.git a2f6ae9abffcba260c22bb235879f0576bf3b783)"}

llvm/test/Transforms/GVN/opt-remarks-non-dominating.ll

+; RUN: opt < %s -gvn -o /dev/null  -pass-remarks-output=%t -S
+; RUN: cat %t | FileCheck %s
+
+; CHECK:      --- !Missed
+; CHECK-NEXT: Pass:            gvn
+; CHECK-NEXT: Name:            LoadClobbered
+; CHECK-NEXT: Function:        nonDominating1
+; CHECK-NEXT: Args:
+; CHECK-NEXT:   - String:          'load of type '
+; CHECK-NEXT:   - Type:            i32
+; CHECK-NEXT:   - String:          ' not eliminated'
+; CHECK-NEXT:   - String:          ' in favor of '
+; CHECK-NEXT:   - OtherAccess:     store
+; CHECK-NEXT:   - String:          ' because it is clobbered by '
+; CHECK-NEXT:   - ClobberedBy:     call
+; CHECK-NEXT: ...
+; CHECK:      --- !Missed
+; CHECK-NEXT: Pass:            gvn
+; CHECK-NEXT: Name:            LoadClobbered
+; CHECK-NEXT: Function:        nonDominating2
+; CHECK-NEXT: Args:
+; CHECK-NEXT:   - String:          'load of type '
+; CHECK-NEXT:   - Type:            i32
+; CHECK-NEXT:   - String:          ' not eliminated'
+; CHECK-NEXT:   - String:          ' in favor of '
+; CHECK-NEXT:   - OtherAccess:     load
+; CHECK-NEXT:   - String:          ' because it is clobbered by '
+; CHECK-NEXT:   - ClobberedBy:     call
+; CHECK-NEXT: ...
+; CHECK:      --- !Missed
+; CHECK-NEXT: Pass:            gvn
+; CHECK-NEXT: Name:            LoadClobbered
+; CHECK-NEXT: Function:        nonDominating2
+; CHECK-NEXT: Args:
+; CHECK-NEXT:   - String:          'load of type '
+; CHECK-NEXT:   - Type:            i32
+; CHECK-NEXT:   - String:          ' not eliminated'
+; CHECK-NEXT:   - String:          ' in favor of '
+; CHECK-NEXT:   - OtherAccess:     load
+; CHECK-NEXT:   - String:          ' because it is clobbered by '
+; CHECK-NEXT:   - ClobberedBy:     call
+; CHECK-NEXT: ...
+; CHECK:      --- !Missed
+; CHECK-NEXT: Pass:            gvn
+; CHECK-NEXT: Name:            LoadClobbered
+; CHECK-NEXT: Function:        nonDominating3
+; CHECK-NEXT: Args:
+; CHECK-NEXT:   - String:          'load of type '
+; CHECK-NEXT:   - Type:            i32
+; CHECK-NEXT:   - String:          ' not eliminated'
+; CHECK-NEXT:   - String:          ' because it is clobbered by '
+; CHECK-NEXT:   - ClobberedBy:     call
+; CHECK-NEXT: ...
+; CHECK:      --- !Missed
+; CHECK-NEXT: Pass:            gvn
+; CHECK-NEXT: Name:            LoadClobbered
+; CHECK-NEXT: Function:        nonDominating4
+; CHECK-NEXT: Args:
+; CHECK-NEXT:   - String:          'load of type '
+; CHECK-NEXT:   - Type:            i32
+; CHECK-NEXT:   - String:          ' not eliminated'
+; CHECK-NEXT:   - String:          ' in favor of '
+; CHECK-NEXT:   - OtherAccess:     store
+; CHECK-NEXT:   - String:          ' because it is clobbered by '
+; CHECK-NEXT:   - ClobberedBy:     call
+; CHECK-NEXT: ...
+
+
+; ModuleID = 'bugpoint-reduced-simplified.bc'
+source_filename = "gvn-test.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Confirm that the partial redundancy being clobbered by the call to
+; clobberingFunc() between store and load is identified.
+
+define dso_local void @nonDominating1(i32* %a, i1 %cond, i32 %b) local_unnamed_addr #0 {
+entry:
+  br i1 %cond, label %if.then, label %if.end
+
+if.then:                                          ; preds = %entry
+  store i32 %b, i32* %a, align 4
+  br label %if.end
+
+if.end:                                           ; preds = %if.then, %entry
+  tail call void @clobberingFunc() #1
+  %0 = load i32, i32* %a, align 4
+  %mul2 = shl nsw i32 %0, 1
+  store i32 %mul2, i32* %a, align 4
+  ret void
+}
+
+declare dso_local void @clobberingFunc() local_unnamed_addr #0
+
+; More complex version of nonDominating1(), this time with loads. The values
+; already loaded into %0 and %1 cannot replace %2 due to clobbering calls.
+; %1 is not clobbered by the first call however, and %0 is irrelevant for the
+; second one since %1 is more recently available.
+
+define dso_local void @nonDominating2(i32* %a, i1 %cond) local_unnamed_addr #0 {
+entry:
+  br i1 %cond, label %if.then, label %if.end5
+
+if.then:                                          ; preds = %entry
+  %0 = load i32, i32* %a, align 4
+  %mul = mul nsw i32 %0, 10
+  tail call void @clobberingFunc() #1
+  %1 = load i32, i32* %a, align 4
+  %mul3 = mul nsw i32 %1, 5
+  tail call void @clobberingFunc() #1
+  br label %if.end5
+
+if.end5:                                          ; preds = %if.then, %entry
+  %2 = load i32, i32* %a, align 4
+  %mul9 = shl nsw i32 %2, 1
+  store i32 %mul9, i32* %a, align 4
+  ret void
+}
+
+; The two stores are both partially available at %0 (were it not for the
+; clobbering call), however neither is strictly more recent than the other, so
+; no attempt is made to identify what value could have potentially been reused
+; otherwise. Just report that the load cannot be eliminated.
+
+define dso_local void @nonDominating3(i32* %a, i32 %b, i32 %c, i1 %cond) local_unnamed_addr #0 {
+entry:
+  br i1 %cond, label %if.end5.sink.split, label %if.else
+
+if.else:                                          ; preds = %entry
+  store i32 %b, i32* %a, align 4
+  br label %if.end5
+
+if.end5.sink.split:                               ; preds = %entry
+  store i32 %c, i32* %a, align 4
+  br label %if.end5
+
+if.end5:                                          ; preds = %if.end5.sink.split, %if.else
+  tail call void @clobberingFunc() #1
+  %0 = load i32, i32* %a, align 4
+  %mul7 = shl nsw i32 %0, 1
+  ret void
+}
+
+; Make sure isPotentiallyReachable() is not called for an instruction
+; outside the current function, as it will cause a crash.
+
+define dso_local void @nonDominating4(i1 %cond, i32 %b) local_unnamed_addr #0 {
+entry:
+  br i1 %cond, label %if.then, label %if.end
+
+if.then:                                          ; preds = %entry
+  store i32 %b, i32* @g, align 4
+  br label %if.end
+
+if.end:                                           ; preds = %if.then, %entry
+  tail call void @clobberingFunc() #1
+  %0 = load i32, i32* @g, align 4
+  %mul2 = shl nsw i32 %0, 1
+  store i32 %mul2, i32* @g, align 4
+  ret void
+}
+
+@g = external global i32
+
+define dso_local void @globalUser(i32 %b) local_unnamed_addr #0 {
+entry:
+  store i32 %b, i32* @g, align 4
+  ret void
+}
+
+attributes #0 = { "use-soft-float"="false" }
+attributes #1 = { nounwind }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 11.0.0 ([e-mail]:hnrklssn/thesis-llvm.git 05da7f0ea394d3994834bb5e387a191388691fc2)"}