[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.
|
@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 from 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.
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.