[GVN] AnalyzeLoadAvailability: Replace a load after lifetime.end with undef (PR20811)

[BK1603]

Summary

This commit attempts on fixing bug #20811 [ https://bugs.llvm.org/show_bug.cgi?id=20811 ]

The commit changes the function AnalyzeLoadAvailability in the following manner:

Currently, the function considers a load after lifetime end to be a instruction clobbered dependency,
and returns false, without populating Res.

The commit makes the function return true if a load is performed after the lifetime of a pointer has ended
and populates Res with undef (just as in the case of a load placed immediately after lifetime start.)

[xbolva00]

Needs a test

[BK1603]

Added test

[fhahn]

Thanks for the patch and sorry for the long response time

llvm/lib/Transforms/Scalar/GVN.cpp

874

I suppose the reason for moving this was that we do not get a Def dependency for lifetime_end?

I think we should keep the original checks at their original positions, we should only get those for defs. The handling of lifetime_end can be moved in the block dealing with clobbers.

[BK1603]

Replaced conditionals at original position.

[BK1603]

@fhahn Sure! Also no worries :)
Thanks for the review.

I've made the changes, can you verify them?

[xbolva00]

llvm/lib/Transforms/Scalar/GVN.cpp

876

if (

[BK1603]

@xbolva00 Couldn't understand what you meant by that comment?
Also after making the changes, should I mark them done, or does the person
who made the comment verifies it and then marks them done?

[xbolva00]

“if(“ is not LLVM-style. Please run clang format on your code changes.

[xbolva00]

should I mark them done

Yes, if you fixed review comments, please mark them as done.

[BK1603]

Refactored code.

[fhahn]

Thanks! Could you also add another test where we have a call to lifetime.end, followed by another defining store, followed by a load? This should not get optimized by this patch.

llvm/lib/Transforms/Scalar/GVN.cpp

873

please remove the unrelated whitespace change.

[hiraditya]

Does NewGVN optimize this already? it will be good to have it there as well.

[BK1603]

Added test case when lifetime end is followed by a defining store. A load in this case should not be
optimized by GVN.

[BK1603]

Removed whitespace.

[BK1603]

In D66604#1668406, @fhahn wrote:

Thanks! Could you also add another test where we have a call to lifetime.end, followed by another defining store, followed by a load? This should not get optimized by this patch.

I have added a test case for it. :)
Also are the name for the test files fine? Or should I change them?

[BK1603]

In D66604#1670556, @hiraditya wrote:

Does NewGVN optimize this already? it will be good to have it there as well.

Sure! :)

Though I think I should wait for this patch to be submitted first, once it is, I can make similar changes to NewGVN.
I hope that is fine?

[xbolva00]

llvm/test/Transforms/GVN/lifetime-end-store.ll

3 ↗

(On Diff #220241)

You can move this test to lifetime-end.ll as “test2”.

llvm/test/Transforms/GVN/lifetime-end.ll

2

You can use llvm/utils/update_test_checks.py to generate CHECK lines for you - much better solution than hand-written check lines.

[BK1603]

Moved test to the same file

[BK1603]

Sorry for the late response ':)

I moved the test to the same file as test2, and well, copy pasted the CHECK lines and the RUN lines for now, I hope its fine?

[xbolva00]

Looks good!

llvm/test/Transforms/GVN/lifetime-end.ll

14

Remove this run line.

[BK1603]

Removed unnecessary RUN line

[xbolva00]

@fhahn , is it fine for you?

@BK1603, should somebody land this patch for you?

[BK1603]

@xbolva00
Yes, i dont have commit access, i need someone to land this patch for me :)

[fhahn]

I can commit it for you in a bit, thanks for the patch! I'll generate the check lines before committing.

[fhahn]

This seems to cause MultiSource/Benchmarks/DOE-ProxyApps-C++/HPCCG/HPCCG and External/SPEC/CINT2000/176.gcc/176.gcc from https://github.com/llvm/test-suite to fail.

[fhahn]

In D66604#1705703, @fhahn wrote:

This seems to cause MultiSource/Benchmarks/DOE-ProxyApps-C++/HPCCG/HPCCG and External/SPEC/CINT2000/176.gcc/176.gcc from https://github.com/llvm/test-suite to fail.

It's not an actual runtime failure, both won't terminate.

[BK1603]

@fhahn

Sorry for the recent slow down in work on this patch ':)
I will check it out tonight and try and fix it!

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;
 }
 
+static bool isLifetimeEnd(const Instruction *Inst) {
+  if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst))
+    return II->getIntrinsicID() == Intrinsic::lifetime_end;
+  return false;
+}
+
 /// 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;
bool GVN::AnalyzeLoadAvailability(LoadInst *LI, MemDepResult DepInfo,
                                   Value *Address, AvailableValue &Res) {
   assert((DepInfo.isDef() || DepInfo.isClobber()) &&
          "expected a local dependence");
   assert(LI->isUnordered() && "rules below are incorrect for ordered access");
 
   const DataLayout &DL = LI->getModule()->getDataLayout();
 
   Instruction *DepInst = DepInfo.getInst();
   if (DepInfo.isClobber()) {
+
+    // Load immediately after lifetime end -> undef
+    if (isLifetimeEnd(DepInst)) {
+      Res = AvailableValue::get(UndefValue::get(LI->getType()));
+      return true;
+    }
+
     // If the dependence is to a store that writes to a superset of the bits
     // read by the load, we can extract the bits we need for the load from the
     // stored value.
     if (StoreInst *DepSI = dyn_cast<StoreInst>(DepInst)) {
       // Can't forward from non-atomic to atomic without violating memory model.
       if (Address && LI->isAtomic() <= DepSI->isAtomic()) {
         int Offset =
           analyzeLoadFromClobberingStore(LI->getType(), Address, DepSI, DL);

llvm/test/Transforms/GVN/lifetime-end.ll

+; RUN: opt < %s -basicaa -gvn -S | FileCheck %s
+
+define i8 @test(i8* %P) nounwind {
+; CHECK: lifetime.end
+; CHECK-NOT: load
+; CHECK:  ret i8 undef
+entry:
+  call void @llvm.lifetime.end(i64 32, i8* %P)
+  %0 = load i8, i8* %P
+  ret i8 %0
+}
+
+define i8 @test2(i8* %P) nounwind {
+; CHECK: lifetime.end
+; CHECK-NOT: laod
+; CHECK: ret i8 3
+entry:
+  call void @llvm.lifetime.end(i64 32, i8* %P)
+  store i8 3, i8* %P
+  %0 = load i8, i8* %P
+  ret i8 %0
+}
+
+declare void @llvm.lifetime.end(i64 %S, i8* nocapture %P)