[clang] 7b54de5 - [funcattrs] Fix a bug in recently introduced writeonly argument inference
Philip Reames via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 3 09:09:52 PST 2021
Author: Philip Reames
Date: 2021-12-03T08:57:15-08:00
New Revision: 7b54de5feffedfc08e5a02d6c9e27c54e3b7f119
URL: https://github.com/llvm/llvm-project/commit/7b54de5feffedfc08e5a02d6c9e27c54e3b7f119
DIFF: https://github.com/llvm/llvm-project/commit/7b54de5feffedfc08e5a02d6c9e27c54e3b7f119.diff
LOG: [funcattrs] Fix a bug in recently introduced writeonly argument inference
This fixes a bug in 740057d. There's two ways to describe the issue:
* One caller hasn't yet proven nocapture on the argument. Given that, the inference routine is responsible for bailing out on a potential capture.
* Even if we know the argument is nocapture, the access inference needs to traverse the exact set of users the capture tracking would (or exit conservatively). Even if capture tracking can prove a store is non-capturing (e.g. to a local alloc which doesn't escape), we still need to track the copy of the pointer to see if it's later reloaded and accessed again.
Note that all the test changes except the newly added ones appear to be false negatives. That is, cases where we could prove writeonly, but the current code isn't strong enough. That's why I didn't spot this originally.
Added:
Modified:
clang/test/CodeGen/ms-mixed-ptr-sizes.c
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
llvm/test/Feature/OperandBundles/pr26510.ll
llvm/test/Transforms/Coroutines/coro-async.ll
llvm/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
llvm/test/Transforms/FunctionAttrs/nocapture.ll
llvm/test/Transforms/FunctionAttrs/readattrs.ll
llvm/test/Transforms/FunctionAttrs/writeonly.ll
Removed:
################################################################################
diff --git a/clang/test/CodeGen/ms-mixed-ptr-sizes.c b/clang/test/CodeGen/ms-mixed-ptr-sizes.c
index 294a8910e13e3..ececa42a4c4dd 100644
--- a/clang/test/CodeGen/ms-mixed-ptr-sizes.c
+++ b/clang/test/CodeGen/ms-mixed-ptr-sizes.c
@@ -7,32 +7,32 @@ struct Foo {
};
void use_foo(struct Foo *f);
void test_sign_ext(struct Foo *f, int * __ptr32 __sptr i) {
-// X64-LABEL: define dso_local void @test_sign_ext({{.*}}i32 addrspace(270)* writeonly %i)
-// X86-LABEL: define dso_local void @test_sign_ext(%struct.Foo* %f, i32* writeonly %i)
+// X64-LABEL: define dso_local void @test_sign_ext({{.*}}i32 addrspace(270)* %i)
+// X86-LABEL: define dso_local void @test_sign_ext(%struct.Foo* %f, i32* %i)
// X64: %{{.+}} = addrspacecast i32 addrspace(270)* %i to i32*
// X86: %{{.+}} = addrspacecast i32* %i to i32 addrspace(272)*
f->p64 = i;
use_foo(f);
}
void test_zero_ext(struct Foo *f, int * __ptr32 __uptr i) {
-// X64-LABEL: define dso_local void @test_zero_ext({{.*}}i32 addrspace(271)* writeonly %i)
-// X86-LABEL: define dso_local void @test_zero_ext({{.*}}i32 addrspace(271)* writeonly %i)
+// X64-LABEL: define dso_local void @test_zero_ext({{.*}}i32 addrspace(271)* %i)
+// X86-LABEL: define dso_local void @test_zero_ext({{.*}}i32 addrspace(271)* %i)
// X64: %{{.+}} = addrspacecast i32 addrspace(271)* %i to i32*
// X86: %{{.+}} = addrspacecast i32 addrspace(271)* %i to i32 addrspace(272)*
f->p64 = i;
use_foo(f);
}
void test_trunc(struct Foo *f, int * __ptr64 i) {
-// X64-LABEL: define dso_local void @test_trunc(%struct.Foo* %f, i32* writeonly %i)
-// X86-LABEL: define dso_local void @test_trunc({{.*}}i32 addrspace(272)* writeonly %i)
+// X64-LABEL: define dso_local void @test_trunc(%struct.Foo* %f, i32* %i)
+// X86-LABEL: define dso_local void @test_trunc({{.*}}i32 addrspace(272)* %i)
// X64: %{{.+}} = addrspacecast i32* %i to i32 addrspace(270)*
// X86: %{{.+}} = addrspacecast i32 addrspace(272)* %i to i32*
f->p32 = i;
use_foo(f);
}
void test_noop(struct Foo *f, int * __ptr32 i) {
-// X64-LABEL: define dso_local void @test_noop({{.*}}i32 addrspace(270)* writeonly %i)
-// X86-LABEL: define dso_local void @test_noop({{.*}}i32* writeonly %i)
+// X64-LABEL: define dso_local void @test_noop({{.*}}i32 addrspace(270)* %i)
+// X86-LABEL: define dso_local void @test_noop({{.*}}i32* %i)
// X64-NOT: addrspacecast
// X86-NOT: addrspacecast
f->p32 = i;
@@ -40,8 +40,8 @@ void test_noop(struct Foo *f, int * __ptr32 i) {
}
void test_other(struct Foo *f, __attribute__((address_space(10))) int *i) {
-// X64-LABEL: define dso_local void @test_other({{.*}}i32 addrspace(10)* writeonly %i)
-// X86-LABEL: define dso_local void @test_other({{.*}}i32 addrspace(10)* writeonly %i)
+// X64-LABEL: define dso_local void @test_other({{.*}}i32 addrspace(10)* %i)
+// X86-LABEL: define dso_local void @test_other({{.*}}i32 addrspace(10)* %i)
// X64: %{{.+}} = addrspacecast i32 addrspace(10)* %i to i32 addrspace(270)*
// X86: %{{.+}} = addrspacecast i32 addrspace(10)* %i to i32*
f->p32 = (int * __ptr32)i;
diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index 1ad0055b56382..2cee9c0b4766a 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -769,6 +769,10 @@ determinePointerAccessAttrs(Argument *A,
break;
case Instruction::Store:
+ if (cast<StoreInst>(I)->getValueOperand() == *U)
+ // untrackable capture
+ return Attribute::None;
+
// A volatile store has side effects beyond what writeonly can be relied
// upon.
if (cast<StoreInst>(I)->isVolatile())
diff --git a/llvm/test/Feature/OperandBundles/pr26510.ll b/llvm/test/Feature/OperandBundles/pr26510.ll
index 878628627b056..1877f35a40067 100644
--- a/llvm/test/Feature/OperandBundles/pr26510.ll
+++ b/llvm/test/Feature/OperandBundles/pr26510.ll
@@ -10,7 +10,7 @@
declare void @foo() readnone
-; CHECK-LABEL: define i8* @test(i8* writeonly %p)
+; CHECK-LABEL: define i8* @test(i8* %p)
; CHECK: %a = alloca i8*, align 8
; CHECK: store i8* %p, i8** %a, align 8
; CHECK: call void @foo() [ "abc"(i8** %a) ]
diff --git a/llvm/test/Transforms/Coroutines/coro-async.ll b/llvm/test/Transforms/Coroutines/coro-async.ll
index 86d95c3366549..151573fe22925 100644
--- a/llvm/test/Transforms/Coroutines/coro-async.ll
+++ b/llvm/test/Transforms/Coroutines/coro-async.ll
@@ -256,7 +256,7 @@ entry:
unreachable
}
-; CHECK-LABEL: define swiftcc void @my_async_function2(%async.task* %task, %async.actor* %actor, i8* writeonly %async.ctxt)
+; CHECK-LABEL: define swiftcc void @my_async_function2(%async.task* %task, %async.actor* %actor, i8* %async.ctxt)
; CHECK-SAME: #[[FRAMEPOINTER:[0-9]+]]
; CHECK-SAME: !dbg ![[SP3:[0-9]+]]
; CHECK: store i8* %async.ctxt,
@@ -269,7 +269,7 @@ entry:
; CHECK: tail call swiftcc void @asyncSuspend(i8* [[CALLEE_CTXT]], %async.task* %task, %async.actor* %actor)
; CHECK: ret void
-; CHECK-LABEL: define internal swiftcc void @my_async_function2.resume.0(i8* writeonly %0, i8* nocapture readnone %1, i8* nocapture readonly %2)
+; CHECK-LABEL: define internal swiftcc void @my_async_function2.resume.0(i8* %0, i8* nocapture readnone %1, i8* nocapture readonly %2)
; CHECK-SAME: #[[FRAMEPOINTER]]
; CHECK-SAME: !dbg ![[SP4:[0-9]+]]
; CHECK: [[CALLEE_CTXT_ADDR:%.*]] = bitcast i8* %2 to i8**
diff --git a/llvm/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll b/llvm/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
index e493787f93b33..435f7810fbde6 100644
--- a/llvm/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
+++ b/llvm/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
@@ -7,7 +7,7 @@ define i32* @a(i32** %p) {
ret i32* %tmp
}
-; CHECK: define i32* @b(i32* writeonly %q)
+; CHECK: define i32* @b(i32* %q)
define i32* @b(i32 *%q) {
%mem = alloca i32*
store i32* %q, i32** %mem
diff --git a/llvm/test/Transforms/FunctionAttrs/nocapture.ll b/llvm/test/Transforms/FunctionAttrs/nocapture.ll
index 5fc33e5c8c192..2042f3e0d724f 100644
--- a/llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ b/llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -8,7 +8,7 @@ define i32* @c1(i32* %q) {
ret i32* %q
}
-; FNATTR: define void @c2(i32* writeonly %q)
+; FNATTR: define void @c2(i32* %q)
; It would also be acceptable to mark %q as readnone. Update @c3 too.
define void @c2(i32* %q) {
store i32* %q, i32** @g
@@ -268,7 +268,7 @@ entry:
}
@g3 = global i8* null
-; FNATTR: define void @captureStrip(i8* writeonly %p)
+; FNATTR: define void @captureStrip(i8* %p)
define void @captureStrip(i8* %p) {
%b = call i8* @llvm.strip.invariant.group.p0i8(i8* %p)
store i8* %b, i8** @g3
diff --git a/llvm/test/Transforms/FunctionAttrs/readattrs.ll b/llvm/test/Transforms/FunctionAttrs/readattrs.ll
index e300b85cf9207..59f58bdee9aa1 100644
--- a/llvm/test/Transforms/FunctionAttrs/readattrs.ll
+++ b/llvm/test/Transforms/FunctionAttrs/readattrs.ll
@@ -34,7 +34,7 @@ define void @test4_2(i8* %p) {
ret void
}
-; CHECK: define void @test5(i8** nocapture writeonly %p, i8* writeonly %q)
+; CHECK: define void @test5(i8** nocapture writeonly %p, i8* %q)
; Missed optz'n: we could make %q readnone, but don't break test6!
define void @test5(i8** %p, i8* %q) {
store i8* %q, i8** %p
@@ -42,7 +42,7 @@ define void @test5(i8** %p, i8* %q) {
}
declare void @test6_1()
-; CHECK: define void @test6_2(i8** nocapture writeonly %p, i8* writeonly %q)
+; CHECK: define void @test6_2(i8** nocapture writeonly %p, i8* %q)
; This is not a missed optz'n.
define void @test6_2(i8** %p, i8* %q) {
store i8* %q, i8** %p
diff --git a/llvm/test/Transforms/FunctionAttrs/writeonly.ll b/llvm/test/Transforms/FunctionAttrs/writeonly.ll
index ac3b5ffcca51b..fb391111b301b 100644
--- a/llvm/test/Transforms/FunctionAttrs/writeonly.ll
+++ b/llvm/test/Transforms/FunctionAttrs/writeonly.ll
@@ -31,6 +31,15 @@ define void @test_store(i8* %p) {
ret void
}
+ at G = external global i8*
+; CHECK: define i8 @test_store_capture(i8* %p)
+define i8 @test_store_capture(i8* %p) {
+ store i8* %p, i8** @G
+ %p2 = load i8*, i8** @G
+ %v = load i8, i8* %p2
+ ret i8 %v
+}
+
; CHECK: define void @test_addressing(i8* nocapture writeonly %p)
define void @test_addressing(i8* %p) {
%gep = getelementptr i8, i8* %p, i64 8
More information about the cfe-commits
mailing list