[BasicAA] Treat returns_twice functions as clobbering unescaped objects #117902
Conversation
Effectively this models all the accesses that occur between the first and second return as happening at the point of the call. I left this a generic check, though at least for setjmp and C semantics specifically, we could skip allocas, as these are required to use volatile accesses when modified between setjmp and longjmp.
llvmbot
added
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Nikita Popov (nikic) ChangesEffectively this models all the accesses that occur between the first and second return as happening at the point of the call. I left this a generic check, though at least for setjmp and C semantics specifically, we could skip allocas, as these are required to use volatile accesses when modified between setjmp and longjmp. Fixes #116668. Full diff: https://github.com/llvm/llvm-project/pull/117902.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index 381fb7bbdb5171..e5f71753005f42 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -947,8 +947,12 @@ ModRefInfo BasicAAResult::getModRefInfo(const CallBase *Call,
//
// Make sure the object has not escaped here, and then check that none of the
// call arguments alias the object below.
+ //
+ // We model calls that can return twice (setjmp) as clobbering non-escaping
+ // objects, to model any accesses that may occur prior to the second return.
if (!isa<Constant>(Object) && Call != Object &&
- AAQI.CA->isNotCapturedBefore(Object, Call, /*OrAt*/ false)) {
+ AAQI.CA->isNotCapturedBefore(Object, Call, /*OrAt*/ false) &&
+ !Call->hasFnAttr(Attribute::ReturnsTwice)) {
// Optimistically assume that call doesn't touch Object and check this
// assumption in the following loop.
diff --git a/llvm/test/Transforms/GVN/setjmp.ll b/llvm/test/Transforms/GVN/setjmp.ll
index 0277fcfa226ed6..0ebe24879d320c 100644
--- a/llvm/test/Transforms/GVN/setjmp.ll
+++ b/llvm/test/Transforms/GVN/setjmp.ll
@@ -5,7 +5,6 @@ declare i32 @setjmp() returns_twice
declare void @longjmp()
declare ptr @malloc(i64)
-; FIXME: This is a miscompile.
define i32 @test() {
; CHECK-LABEL: define i32 @test() {
; CHECK-NEXT: [[MALLOC:%.*]] = call noalias ptr @malloc(i64 4)
@@ -18,7 +17,8 @@ define i32 @test() {
; CHECK-NEXT: call void @longjmp()
; CHECK-NEXT: unreachable
; CHECK: [[IF_END]]:
-; CHECK-NEXT: ret i32 10
+; CHECK-NEXT: [[RES:%.*]] = load i32, ptr [[MALLOC]], align 4
+; CHECK-NEXT: ret i32 [[RES]]
;
%malloc = call noalias ptr @malloc(i64 4)
store i32 10, ptr %malloc, align 4
|
|
@llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesEffectively this models all the accesses that occur between the first and second return as happening at the point of the call. I left this a generic check, though at least for setjmp and C semantics specifically, we could skip allocas, as these are required to use volatile accesses when modified between setjmp and longjmp. Fixes #116668. Full diff: https://github.com/llvm/llvm-project/pull/117902.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index 381fb7bbdb5171..e5f71753005f42 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -947,8 +947,12 @@ ModRefInfo BasicAAResult::getModRefInfo(const CallBase *Call,
//
// Make sure the object has not escaped here, and then check that none of the
// call arguments alias the object below.
+ //
+ // We model calls that can return twice (setjmp) as clobbering non-escaping
+ // objects, to model any accesses that may occur prior to the second return.
if (!isa<Constant>(Object) && Call != Object &&
- AAQI.CA->isNotCapturedBefore(Object, Call, /*OrAt*/ false)) {
+ AAQI.CA->isNotCapturedBefore(Object, Call, /*OrAt*/ false) &&
+ !Call->hasFnAttr(Attribute::ReturnsTwice)) {
// Optimistically assume that call doesn't touch Object and check this
// assumption in the following loop.
diff --git a/llvm/test/Transforms/GVN/setjmp.ll b/llvm/test/Transforms/GVN/setjmp.ll
index 0277fcfa226ed6..0ebe24879d320c 100644
--- a/llvm/test/Transforms/GVN/setjmp.ll
+++ b/llvm/test/Transforms/GVN/setjmp.ll
@@ -5,7 +5,6 @@ declare i32 @setjmp() returns_twice
declare void @longjmp()
declare ptr @malloc(i64)
-; FIXME: This is a miscompile.
define i32 @test() {
; CHECK-LABEL: define i32 @test() {
; CHECK-NEXT: [[MALLOC:%.*]] = call noalias ptr @malloc(i64 4)
@@ -18,7 +17,8 @@ define i32 @test() {
; CHECK-NEXT: call void @longjmp()
; CHECK-NEXT: unreachable
; CHECK: [[IF_END]]:
-; CHECK-NEXT: ret i32 10
+; CHECK-NEXT: [[RES:%.*]] = load i32, ptr [[MALLOC]], align 4
+; CHECK-NEXT: ret i32 [[RES]]
;
%malloc = call noalias ptr @malloc(i64 4)
store i32 10, ptr %malloc, align 4
|
|
Unfortunately this broke some Objective C tests, which appears to rely on optimizations still happening for allocas. It inserts its own optimization barriers using inline asm. So it looks like we do have to special case the alloca case if we want to preserve those Objective C optimizations. It's somewhat iffy though, because LLVM can promote globals to allocas. I'd be happy to adjust the tests with the optimization regressions instead... |
There was a problem hiding this comment.
The whole "setjmp doesn't have to preserve local variables" thing is an artifact of ancient C compiler technology where the compiler didn't actually understand setjmp semantics. Given a modern setjmp-aware compiler, it shouldn't be a thing; we have the ability to correctly preserve local variables.
That said, given the current state of returns_twice, carving out an exception for alloca seems okay.
|
@nikic Do you believe we should also be checking for returns_twice in callCapturesBefore(), along the following lines? llvm-project/llvm/lib/Analysis/AliasAnalysis.cpp Lines 620 to 626 in f9f62ef AFAICT, if we are given a non-escaping memory location and the given call-site is returns_twice, we should continue conservatively say the call may clobber this location. |
|
Possibly. Note that callCapturesBefore() is mostly obsolete and only used by one place in MemCpyOpt at this point, so it likely doesn't make a difference in practice. |
Thanks for the elucidation. I'll consider opening a patch. GVN MemorySSA main change would become a user of callCapturesBefore() as well. I imagine we may want to migrate these APIs to getModRefInfo()/getCapturesBefore() sometime soon. |
|
@antoniofrighetto GVN should not be using callCapturesBefore(). It should be using BatchAA with EarliestEscapeAnalysis. |
|
@nikic Sorry for getting back again into this. I moved to using EarliestEscapeAnalysis::getCapturesBefore() and moved the returns_twice check close to the caller (in GVN). However, I'm still not clear. If getCapturesBefore() is passed with OrAt true, and the instruction which may capture the object (thus, instruction itself included) is our setjmp, then shouldn't we expect the routine to say the provenance of the underlying object (a noalias malloc ptr in this case, as per the test in the PR) may be captured by the setjmp? (For this instance, we say CaptureComponents::None). If that happens to be the case, should returns_twice be taken into account within IsNotCapturedBefore lambda? Anyway, perhaps this discussion should be held in the code review of the MemSSA main patch. |
|
Oh, scratch the above. getCapturesBefore is about pointer provenance, not about clobbering memory. |
|
I expect you just want getModRefInfo(), which uses getCapturesBefore() when appropriate. |
I'm only left with a doubt: being mostly obsolete, is there any particular reason why MemCpyOpt is not leveraging BatchAA with EarliestEscapeAnalysis? Or should it be possible (desirable) to move MemCpyOpt to employ the latter as well? |
|
MemCpyOpt already uses BatchAA with EarliestEscapeAnalysis. I tried dropping callCapturesBefore() in #110484, but there were enough regressions on llvm-opt-benchmarks that I decided to keep it for now. (We could remove callCapturesBefore while keeping its additional precision by adding a CaptureAnalysis using CapturesBefore, possibly in conjunction with support for "chained" capture analysis where we try EarliestEscapeAnalysis before falling back to CatpuresBeforeAnalysis.) |
|
Oops, sorry, I wonder which file I was looking at.
Right, this makes sense, thanks for direction. With chained capture analysis, I assume we may want a ChainedCaptureAnalysis instance, to be passed to BatchAA, which is delegated to own the two EEA and CBA objects. Also, IIUC, we would not be interested in the mod/ref refinement done per-argument in callCapturesBefore() anymore – yet, if CBA is passed with OrAt, we should still be checking for possible pointer escaping at call-site level via return value. |
4 participants
Effectively this models all the accesses that occur between the first and second return as happening at the point of the call.
I left this a generic check, though at least for setjmp and C semantics specifically, we could skip allocas, as these are required to use volatile accesses when modified between setjmp and longjmp.
Fixes #116668.