reames created this revision.
reames added reviewers: jdoerfert, anna, sstefan1, aeubanks, modimo.
Herald added subscribers: ormris, bollu, hiraditya, mcrosier.
reames requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.
This is a rewrite of the existing code. It isn't even close to NFC as it fixes
several problems with the existing code. Changes detailed below.
1. Argument attributes on an indirect call site are honored. We'd previously
only been honoring these on unknown direct calls, which is just weird.
2. Consistently treat calling a function pointer as an operation which reads
the function pointer. Previously, we gave it different treatment based on
which attributes were on the callsite. (e.g. a readonly callsite vs a
writeonly callsite changed the intepretation of the function pointer access for
an indirect call)
3. Honor attributes on vararg function declarations when visiting a vararg
argument. (Previously, we aborted the entire analysis.)
In addition to the previously described functional changes, the resulting code
is both shorter, and easier to follow.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D115021
Files:
clang/test/CodeGen/arm-cmse-attr.c
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
llvm/test/Transforms/FunctionAttrs/nocapture.ll
llvm/test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
llvm/test/Transforms/FunctionAttrs/writeonly.ll
Index: llvm/test/Transforms/FunctionAttrs/writeonly.ll
===================================================================
--- llvm/test/Transforms/FunctionAttrs/writeonly.ll
+++ llvm/test/Transforms/FunctionAttrs/writeonly.ll
@@ -83,19 +83,19 @@
ret void
}
-; CHECK: define void @fptr_test1(i8* %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test1(i8* %p, void (i8*)* nocapture readonly %f)
define void @fptr_test1(i8* %p, void (i8*)* %f) {
call void %f(i8* %p)
ret void
}
-; CHECK: define void @fptr_test2(i8* %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test2(i8* writeonly %p, void (i8*)* nocapture readonly %f)
define void @fptr_test2(i8* %p, void (i8*)* %f) {
call void %f(i8* writeonly %p)
ret void
}
-; CHECK: define void @fptr_test3(i8* writeonly %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test3(i8* writeonly %p, void (i8*)* nocapture readonly %f)
define void @fptr_test3(i8* %p, void (i8*)* %f) {
call void %f(i8* %p) writeonly
ret void
Index: llvm/test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
===================================================================
--- llvm/test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
+++ llvm/test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
@@ -1,14 +1,8 @@
; RUN: opt -function-attrs -S < %s | FileCheck %s
; RUN: opt -passes=function-attrs -S < %s | FileCheck %s
-; This checks for an iterator wraparound bug in FunctionAttrs. The previous
-; "incorrect" behavior was inferring readonly for the %x argument in @caller.
-; Inferring readonly for %x *is* actually correct, since @va_func is marked
-; readonly, but FunctionAttrs was inferring readonly for the wrong reasons (and
-; we _need_ the readonly on @va_func to trigger the problematic code path). It
-; is possible that in the future FunctionAttrs becomes smart enough to infer
-; readonly for %x for the right reasons, and at that point this test will have
-; to be marked invalid.
+; This checks for a previously existing iterator wraparound bug in
+; FunctionAttrs, and in the process covers corner cases with varargs.
declare void @llvm.va_start(i8*)
declare void @llvm.va_end(i8*)
@@ -24,8 +18,26 @@
}
define i32 @caller(i32* %x) {
-; CHECK-LABEL: define i32 @caller(i32* nocapture %x)
+; CHECK-LABEL: define i32 @caller(i32* nocapture readonly %x)
entry:
call void(i32*,...) @va_func(i32* null, i32 0, i32 0, i32 0, i32* %x)
ret i32 42
}
+
+define void @va_func2(i32* readonly %b, ...) {
+; CHECK-LABEL: define void @va_func2(i32* nocapture readonly %b, ...)
+ entry:
+ %valist = alloca i8
+ call void @llvm.va_start(i8* %valist)
+ call void @llvm.va_end(i8* %valist)
+ %x = call i32 @caller(i32* %b)
+ ret void
+}
+
+define i32 @caller2(i32* %x, i32* %y) {
+; CHECK-LABEL: define i32 @caller2(i32* nocapture readonly %x, i32* %y)
+ entry:
+ call void(i32*,...) @va_func2(i32* %x, i32 0, i32 0, i32 0, i32* %y)
+ ret i32 42
+}
+
Index: llvm/test/Transforms/FunctionAttrs/nocapture.ll
===================================================================
--- llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -128,7 +128,7 @@
}
-; FNATTR: define void @nc3(void ()* nocapture %p)
+; FNATTR: define void @nc3(void ()* nocapture readonly %p)
define void @nc3(void ()* %p) {
call void %p()
ret void
@@ -141,7 +141,7 @@
ret void
}
-; FNATTR: define void @nc5(void (i8*)* nocapture %f, i8* nocapture %p)
+; FNATTR: define void @nc5(void (i8*)* nocapture readonly %f, i8* nocapture %p)
define void @nc5(void (i8*)* %f, i8* %p) {
call void %f(i8* %p) readonly nounwind
call void %f(i8* nocapture %p)
Index: llvm/lib/Transforms/IPO/FunctionAttrs.cpp
===================================================================
--- llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -707,62 +707,56 @@
continue;
}
- Function *F = CB.getCalledFunction();
- if (!F) {
- if (CB.onlyReadsMemory()) {
- IsRead = true;
- AddUsersToWorklistIfCapturing();
- continue;
- } else if (!CB.isCallee(U) && CB.hasFnAttr(Attribute::WriteOnly)) {
- IsWrite = true;
- AddUsersToWorklistIfCapturing();
- continue;
- }
- return Attribute::None;
+ if (CB.isCallee(U)) {
+ // Indirect call of an argument is a reading, noncapturing use
+ IsRead = true;
+ Captures = false;
+ AddUsersToWorklistIfCapturing();
+ continue;
}
+ // U cannot be the callee operand use as we explicitly checked for that.
// Note: the callee and the two successor blocks *follow* the argument
// operands. This means there is no need to adjust UseIndex to account
// for these.
-
- unsigned UseIndex = std::distance(CB.arg_begin(), U);
-
- // U cannot be the callee operand use: since we're exploring the
- // transitive uses of an Argument, having such a use be a callee would
- // imply the call site is an indirect call or invoke; and we'd take the
- // early exit above.
+ const unsigned UseIndex = std::distance(CB.arg_begin(), U);
assert(UseIndex < CB.data_operands_size() &&
"Data operand use expected!");
- bool IsOperandBundleUse = UseIndex >= CB.arg_size();
-
- if (UseIndex >= F->arg_size() && !IsOperandBundleUse) {
- assert(F->isVarArg() && "More params than args in non-varargs call");
- return Attribute::None;
- }
-
Captures &= !CB.doesNotCapture(UseIndex);
- // Since the optimizer (by design) cannot see the data flow corresponding
- // to a operand bundle use, these cannot participate in the optimistic SCC
- // analysis. Instead, we model the operand bundle uses as arguments in
- // call to a function external to the SCC.
- if (IsOperandBundleUse ||
- !SCCNodes.count(&*std::next(F->arg_begin(), UseIndex))) {
-
- // The accessors used on call site here do the right thing for calls and
- // invokes with operand bundles.
-
- if (CB.doesNotAccessMemory(UseIndex)) { /* nop */ }
- else if (CB.onlyReadsMemory() || CB.onlyReadsMemory(UseIndex))
- IsRead = true;
- else if (CB.hasFnAttr(Attribute::WriteOnly) ||
- CB.dataOperandHasImpliedAttr(UseIndex, Attribute::WriteOnly))
- IsWrite = true;
- else
- return Attribute::None;
+ auto canSpeculate = [&]() {
+ if (UseIndex >= CB.arg_size())
+ // Since the optimizer (by design) cannot see the data flow
+ // corresponding to a operand bundle use, these cannot participate in
+ // the optimistic SCC analysis. Instead, we model the operand bundle
+ // uses as arguments in call to a function external to the SCC.
+ return false;
+ auto *F = CB.getCalledFunction();
+ if (!F)
+ return false;
+ if (UseIndex >= F->arg_size()) {
+ assert(F->isVarArg());
+ return false;
+ }
+ Argument *A = &*std::next(F->arg_begin(), UseIndex);
+ return 0 != SCCNodes.count(A);
+ };
+ if (canSpeculate()) {
+ // Speculatively ignore this use for now.
+ AddUsersToWorklistIfCapturing();
+ continue;
}
+ if (CB.doesNotAccessMemory(UseIndex)) { /* nop */ }
+ else if (CB.onlyReadsMemory() || CB.onlyReadsMemory(UseIndex))
+ IsRead = true;
+ else if (CB.hasFnAttr(Attribute::WriteOnly) ||
+ CB.dataOperandHasImpliedAttr(UseIndex, Attribute::WriteOnly))
+ IsWrite = true;
+ else
+ return Attribute::None;
+
AddUsersToWorklistIfCapturing();
break;
}
Index: clang/test/CodeGen/arm-cmse-attr.c
===================================================================
--- clang/test/CodeGen/arm-cmse-attr.c
+++ clang/test/CodeGen/arm-cmse-attr.c
@@ -29,9 +29,9 @@
{
}
-// CHECK: define{{.*}} void @f1(void ()* nocapture %fptr) {{[^#]*}}#0 {
+// CHECK: define{{.*}} void @f1(void ()* nocapture readonly %fptr) {{[^#]*}}#0 {
// CHECK: call void %fptr() #2
-// CHECK: define{{.*}} void @f2(void ()* nocapture %fptr) {{[^#]*}}#0 {
+// CHECK: define{{.*}} void @f2(void ()* nocapture readonly %fptr) {{[^#]*}}#0 {
// CHECK: call void %fptr() #2
// CHECK: define{{.*}} void @f3() {{[^#]*}}#1 {
// CHECK: define{{.*}} void @f4() {{[^#]*}}#1 {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
