[TailCallElim] Don’t mark llvm.stackrestore with tail-call #101352
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
llvm:analysis
|
@llvm/pr-subscribers-llvm-transforms Author: Shimin Cui (scui-ibm) Changes In BasicAAResult::getModRefInfo, the special handling of alloca/stackrestore is after checking whether the call is a tailcall or not. This is to move the specific stackrestore code before the tail call checking, as the call to stackrestore can be marked as a tail call by tailcallelim which is performed before MemCpyOpt in the transformation pipeline. Full diff: https://github.com/llvm/llvm-project/pull/101352.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index e474899fb548e..504026e44f531 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -914,6 +914,12 @@ ModRefInfo BasicAAResult::getModRefInfo(const CallBase *Call,
const Value *Object = getUnderlyingObject(Loc.Ptr);
+ // Stack restore is able to modify unescaped dynamic allocas. Assume it may
+ // modify them even though the alloca is not escaped.
+ if (auto *AI = dyn_cast<AllocaInst>(Object))
+ if (!AI->isStaticAlloca() && isIntrinsicCall(Call, Intrinsic::stackrestore))
+ return ModRefInfo::Mod;
+
// Calls marked 'tail' cannot read or write allocas from the current frame
// because the current frame might be destroyed by the time they run. However,
// a tail call may use an alloca with byval. Calling with byval copies the
@@ -925,12 +931,6 @@ ModRefInfo BasicAAResult::getModRefInfo(const CallBase *Call,
!CI->getAttributes().hasAttrSomewhere(Attribute::ByVal))
return ModRefInfo::NoModRef;
- // Stack restore is able to modify unescaped dynamic allocas. Assume it may
- // modify them even though the alloca is not escaped.
- if (auto *AI = dyn_cast<AllocaInst>(Object))
- if (!AI->isStaticAlloca() && isIntrinsicCall(Call, Intrinsic::stackrestore))
- return ModRefInfo::Mod;
-
// A call can access a locally allocated object either because it is passed as
// an argument to the call, or because it has escaped prior to the call.
//
diff --git a/llvm/test/Transforms/MemCpyOpt/stackrestore.ll b/llvm/test/Transforms/MemCpyOpt/stackrestore.ll
index 0fc37c44fa9e8..f5feea98e21cd 100644
--- a/llvm/test/Transforms/MemCpyOpt/stackrestore.ll
+++ b/llvm/test/Transforms/MemCpyOpt/stackrestore.ll
@@ -1,5 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -S -passes=memcpyopt < %s -verify-memoryssa | FileCheck %s
+; RUN: opt -S -passes="tailcallelim,memcpyopt" < %s -verify-memoryssa | FileCheck %s
; PR40118: BasicAA didn't realize that stackrestore ends the lifetime of
; unescaped dynamic allocas, such as those that might come from inalloca.
@@ -23,7 +24,7 @@ define i32 @test_norestore(i32 %n) {
; CHECK-NEXT: store i8 0, ptr [[P10]], align 1
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr [[TMPMEM]], ptr [[P]], i32 10, i1 false)
; CHECK-NEXT: call void @external()
-; CHECK-NEXT: [[HEAP:%.*]] = call ptr @malloc(i32 9)
+; CHECK-NEXT: [[HEAP:%.*]] = tail call ptr @malloc(i32 9)
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr [[HEAP]], ptr align 1 @str, i32 9, i1 false)
; CHECK-NEXT: call void @useit(ptr [[HEAP]])
; CHECK-NEXT: ret i32 0
@@ -40,7 +41,7 @@ define i32 @test_norestore(i32 %n) {
call void @llvm.memcpy.p0.p0.i32(ptr %tmpmem, ptr %p, i32 10, i1 false)
call void @external()
- %heap = call ptr @malloc(i32 9)
+ %heap = tail call ptr @malloc(i32 9)
call void @llvm.memcpy.p0.p0.i32(ptr %heap, ptr %tmpmem, i32 9, i1 false)
call void @useit(ptr %heap)
ret i32 0
@@ -58,7 +59,7 @@ define i32 @test_stackrestore() {
; CHECK-NEXT: store i8 0, ptr [[P10]], align 1
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr [[TMPMEM]], ptr [[ARGMEM]], i32 10, i1 false)
; CHECK-NEXT: call void @llvm.stackrestore.p0(ptr [[INALLOCA_SAVE]])
-; CHECK-NEXT: [[HEAP:%.*]] = call ptr @malloc(i32 9)
+; CHECK-NEXT: [[HEAP:%.*]] = tail call ptr @malloc(i32 9)
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr [[HEAP]], ptr [[TMPMEM]], i32 9, i1 false)
; CHECK-NEXT: call void @useit(ptr [[HEAP]])
; CHECK-NEXT: ret i32 0
@@ -74,7 +75,7 @@ define i32 @test_stackrestore() {
call void @llvm.memcpy.p0.p0.i32(ptr %tmpmem, ptr %argmem, i32 10, i1 false)
call void @llvm.stackrestore(ptr %inalloca.save)
- %heap = call ptr @malloc(i32 9)
+ %heap = tail call ptr @malloc(i32 9)
call void @llvm.memcpy.p0.p0.i32(ptr %heap, ptr %tmpmem, i32 9, i1 false)
call void @useit(ptr %heap)
ret i32 0
|
|
@llvm/pr-subscribers-llvm-analysis Author: Shimin Cui (scui-ibm) Changes In BasicAAResult::getModRefInfo, the special handling of alloca/stackrestore is after checking whether the call is a tailcall or not. This is to move the specific stackrestore code before the tail call checking, as the call to stackrestore can be marked as a tail call by tailcallelim which is performed before MemCpyOpt in the transformation pipeline. Full diff: https://github.com/llvm/llvm-project/pull/101352.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index e474899fb548e..504026e44f531 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -914,6 +914,12 @@ ModRefInfo BasicAAResult::getModRefInfo(const CallBase *Call,
const Value *Object = getUnderlyingObject(Loc.Ptr);
+ // Stack restore is able to modify unescaped dynamic allocas. Assume it may
+ // modify them even though the alloca is not escaped.
+ if (auto *AI = dyn_cast<AllocaInst>(Object))
+ if (!AI->isStaticAlloca() && isIntrinsicCall(Call, Intrinsic::stackrestore))
+ return ModRefInfo::Mod;
+
// Calls marked 'tail' cannot read or write allocas from the current frame
// because the current frame might be destroyed by the time they run. However,
// a tail call may use an alloca with byval. Calling with byval copies the
@@ -925,12 +931,6 @@ ModRefInfo BasicAAResult::getModRefInfo(const CallBase *Call,
!CI->getAttributes().hasAttrSomewhere(Attribute::ByVal))
return ModRefInfo::NoModRef;
- // Stack restore is able to modify unescaped dynamic allocas. Assume it may
- // modify them even though the alloca is not escaped.
- if (auto *AI = dyn_cast<AllocaInst>(Object))
- if (!AI->isStaticAlloca() && isIntrinsicCall(Call, Intrinsic::stackrestore))
- return ModRefInfo::Mod;
-
// A call can access a locally allocated object either because it is passed as
// an argument to the call, or because it has escaped prior to the call.
//
diff --git a/llvm/test/Transforms/MemCpyOpt/stackrestore.ll b/llvm/test/Transforms/MemCpyOpt/stackrestore.ll
index 0fc37c44fa9e8..f5feea98e21cd 100644
--- a/llvm/test/Transforms/MemCpyOpt/stackrestore.ll
+++ b/llvm/test/Transforms/MemCpyOpt/stackrestore.ll
@@ -1,5 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -S -passes=memcpyopt < %s -verify-memoryssa | FileCheck %s
+; RUN: opt -S -passes="tailcallelim,memcpyopt" < %s -verify-memoryssa | FileCheck %s
; PR40118: BasicAA didn't realize that stackrestore ends the lifetime of
; unescaped dynamic allocas, such as those that might come from inalloca.
@@ -23,7 +24,7 @@ define i32 @test_norestore(i32 %n) {
; CHECK-NEXT: store i8 0, ptr [[P10]], align 1
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr [[TMPMEM]], ptr [[P]], i32 10, i1 false)
; CHECK-NEXT: call void @external()
-; CHECK-NEXT: [[HEAP:%.*]] = call ptr @malloc(i32 9)
+; CHECK-NEXT: [[HEAP:%.*]] = tail call ptr @malloc(i32 9)
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr [[HEAP]], ptr align 1 @str, i32 9, i1 false)
; CHECK-NEXT: call void @useit(ptr [[HEAP]])
; CHECK-NEXT: ret i32 0
@@ -40,7 +41,7 @@ define i32 @test_norestore(i32 %n) {
call void @llvm.memcpy.p0.p0.i32(ptr %tmpmem, ptr %p, i32 10, i1 false)
call void @external()
- %heap = call ptr @malloc(i32 9)
+ %heap = tail call ptr @malloc(i32 9)
call void @llvm.memcpy.p0.p0.i32(ptr %heap, ptr %tmpmem, i32 9, i1 false)
call void @useit(ptr %heap)
ret i32 0
@@ -58,7 +59,7 @@ define i32 @test_stackrestore() {
; CHECK-NEXT: store i8 0, ptr [[P10]], align 1
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr [[TMPMEM]], ptr [[ARGMEM]], i32 10, i1 false)
; CHECK-NEXT: call void @llvm.stackrestore.p0(ptr [[INALLOCA_SAVE]])
-; CHECK-NEXT: [[HEAP:%.*]] = call ptr @malloc(i32 9)
+; CHECK-NEXT: [[HEAP:%.*]] = tail call ptr @malloc(i32 9)
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr [[HEAP]], ptr [[TMPMEM]], i32 9, i1 false)
; CHECK-NEXT: call void @useit(ptr [[HEAP]])
; CHECK-NEXT: ret i32 0
@@ -74,7 +75,7 @@ define i32 @test_stackrestore() {
call void @llvm.memcpy.p0.p0.i32(ptr %tmpmem, ptr %argmem, i32 10, i1 false)
call void @llvm.stackrestore(ptr %inalloca.save)
- %heap = call ptr @malloc(i32 9)
+ %heap = tail call ptr @malloc(i32 9)
call void @llvm.memcpy.p0.p0.i32(ptr %heap, ptr %tmpmem, i32 9, i1 false)
call void @useit(ptr %heap)
ret i32 0
|
nikic
reviewed
Aug 1, 2024
There was a problem hiding this comment.
The alternative here is that we declare the tailcallelim transform on llvm.stackrestore illegal. Which I think is a better idea in case anything else looks at the "tail" marker.
Agree. LangRef says this:
Both markers imply that the callee does not access allocas from the caller.
Where "both markers" are tail/musttail here. This requirement is not satisfied for stackrestore, so it should not get a tail marker.
|
Thanks to @efriedma-quic and @nikic . I'll make a change in tailcallelim instead for this |
|
Can you please update the PR title and description? |
nikic
reviewed
Aug 2, 2024
scui-ibm
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is to teach tailcallelim transformation not to mark llvm.stackrestore with tail-call, as the intrinsic call can modify unescaped allocas from the caller.
This fixes a problem found with our downstream testing. The problem can also be shown when running the test case llvm/test/Transforms/MemCpyOpt/stackrestore with passes=”tailcallelim, memcpyopt”.