[GVN] Bug fix to reportMayClobberedLoad remark

[virnarula]

Summary

Bug fix to avoid assert crashing when generating remarks for GVN crashing.

Intention of assert is correct but ignores edge case of instructions being equivalent.

Reduced input that causes crash when remarks are turned on:

target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
target triple = "arm64-apple-macosx12.0.0"

define ptr @ReplaceWithTidy(ptr %zz_hold) {
cond.end480.us:
  %0 = load ptr, ptr null, align 8
  store ptr %0, ptr %0, align 8
  store ptr null, ptr %zz_hold, align 8
  %1 = load ptr, ptr %0, align 8
  store ptr %1, ptr null, align 8
  ret ptr null
}

[fhahn]

Could please include the test case in the patch?

[virnarula]

Add testing

[fhahn]

Thanks for the update! It looks like the changes in GVN.cpp got dropped though

llvm/test/Transforms/GVN/remarks-selfdomination.ll

6

this is not needed

18

Could you updatee the name here to entry or something similar?

19

load from null is UB in LLVM IR. Could you instead add a ptr argument to the function and use it instead of null? Same for the store. It might be good to simplify the name of zz_hold as well.

[virnarula]

Add test to patch

[virnarula]

update test variable names

[fhahn]

LGTM, thanks for the fix!

llvm/lib/Transforms/Scalar/GVN.cpp

for (auto *U : Load->getPointerOperand()->users()) {
     if (U != Load && (isa<LoadInst>(U) || isa<StoreInst>(U)) &&
         cast<Instruction>(U)->getFunction() == Load->getFunction() &&
         DT->dominates(cast<Instruction>(U), Load)) {
       // 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),
+          assert(U == OtherAccess || 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 Load.

llvm/test/Transforms/GVN/remarks-selfdomination.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -gvn -pass-remarks-analysis=gvn -S < %s | FileCheck %s
+
+define ptr @ReplaceWithTidy(ptr %one, ptr %two, ptr %three) {
+; CHECK-LABEL: @ReplaceWithTidy(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = load ptr, ptr [[ONE:%.*]], align 8
+; CHECK-NEXT:    store ptr [[TMP0]], ptr [[TMP0]], align 8
+; CHECK-NEXT:    store ptr null, ptr [[TWO:%.*]], align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = load ptr, ptr [[TMP0]], align 8
+; CHECK-NEXT:    store ptr [[TMP1]], ptr [[THREE:%.*]], align 8
+; CHECK-NEXT:    ret ptr null
+;
+entry:
+  %0 = load ptr, ptr %one, align 8
+  store ptr %0, ptr %0, align 8
+  store ptr null, ptr %two, align 8
+  %1 = load ptr, ptr %0, align 8
+  store ptr %1, ptr %three, align 8
+  ret ptr null
+}