[PATCH] D149563: [TRE] don't escape callsite if its alloca argument is readonly

[Joshua Cao via Phabricator via llvm-commits]

caojoshua created this revision.
Herald added subscribers: laytonio, hiraditya.
Herald added a project: All.
caojoshua added reviewers: avl, efriedma, Carrot.
caojoshua updated this revision to Diff 518372.
caojoshua edited the summary of this revision.
caojoshua added a comment.
caojoshua published this revision for review.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Update commit message


caojoshua added a comment.

As a side note, while looking through the code,  I didn't understand this:

  // If it can write to memory, it can leak the alloca value.

Even if an alloca is written to in a called function, I don't see why the callsite needs to be escaped. I also don't see why we care about the alloca's value, we only care about the pointer to the alloca. I think maybe the comment is concerned about the called function writing copying the alloca pointer to a global for example, but that we should not be concerned because we already checked for `nocapture`


Prior to this patch, we were escaping an alloca if the called function
writes to any memory. In this patch, we only check that the alloca
argument is not written to.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149563

Files:
  llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
  llvm/test/Transforms/TailCallElim/basic.ll


Index: llvm/test/Transforms/TailCallElim/basic.ll
===================================================================
--- llvm/test/Transforms/TailCallElim/basic.ll
+++ llvm/test/Transforms/TailCallElim/basic.ll
@@ -246,7 +246,7 @@
 ; Alloca's that are readonly arguments should not be be escaped.
 define void @test16() {
 ; CHECK-LABEL: @test16
-; CHECK-NOT: tail call void @noarg
+; CHECK: tail call void @noarg
 entry:
   %x = alloca i32, align 4
   call void @use_readonly(ptr %x)
Index: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
+++ llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
@@ -133,14 +133,17 @@
         // beyond the lifetime of the current frame.
         if (CB.isArgOperand(U) && CB.isByValArgument(CB.getArgOperandNo(U)))
           continue;
-        bool IsNocapture =
-            CB.isDataOperand(U) && CB.doesNotCapture(CB.getDataOperandNo(U));
-        callUsesLocalStack(CB, IsNocapture);
-        if (IsNocapture) {
+        AllocaUsers.insert(&CB);
+        if (!CB.isDataOperand(U))
+          continue;
+        unsigned OperandNo = CB.getDataOperandNo(U);
+        if (CB.doesNotCapture(OperandNo))
           // If the alloca-derived argument is passed in as nocapture, then it
           // can't propagate to the call's return. That would be capturing.
           continue;
-        }
+        if (!CB.onlyReadsMemory(CB.getDataOperandNo(U)))
+          // If it can write to memory, it can leak the alloca value.
+          EscapePoints.insert(&CB);
         break;
       }
       case Instruction::Load: {
@@ -168,19 +171,6 @@
     }
   }
 
-  void callUsesLocalStack(CallBase &CB, bool IsNocapture) {
-    // Add it to the list of alloca users.
-    AllocaUsers.insert(&CB);
-
-    // If it's nocapture then it can't capture this alloca.
-    if (IsNocapture)
-      return;
-
-    // If it can write to memory, it can leak the alloca value.
-    if (!CB.onlyReadsMemory())
-      EscapePoints.insert(&CB);
-  }
-
   SmallPtrSet<Instruction *, 32> AllocaUsers;
   SmallPtrSet<Instruction *, 32> EscapePoints;
 };


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D149563.518372.patch
Type: text/x-patch
Size: 2201 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230501/b6ce65ed/attachment.bin>