[llvm] 1fee719 - [funcattrs] Fix incorrect readnone/readonly inference on captured arguments
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 21 09:34:48 PST 2021
Author: Philip Reames
Date: 2021-12-21T09:34:14-08:00
New Revision: 1fee7195c99a3cbeabb1ade35b00d974b4829e58
URL: https://github.com/llvm/llvm-project/commit/1fee7195c99a3cbeabb1ade35b00d974b4829e58
DIFF: https://github.com/llvm/llvm-project/commit/1fee7195c99a3cbeabb1ade35b00d974b4829e58.diff
LOG: [funcattrs] Fix incorrect readnone/readonly inference on captured arguments
This fixes a bug where we would infer readnone/readonly for a function which passed a value to a function which could capture it. With the value captured in memory, the function could reload the value from memory after the call, and write to it. Inferring the argument as readnone or readonly is unsound.
@jdoerfert apparently noticed this about two years ago, and tests were checked in with 76467c4, but the issue appears to have never gotten fixed.
Since this seems like this issue should break everything, let me explain why the case is actually fairly narrow. The main inference loop over the argument SCCs only analyzes nocapture arguments. As such, we can only hit this when construction the partial SCCs. Due to that restriction, we can only hit this when we have either a) a function declaration with a manually annotated argument, or b) an immediately self recursive call.
It's also worth highlighting that we do have cases we can infer readonly/readnone on a capturing argument validly. The easiest example is a function which simply returns its argument without ever accessing it.
Differential Revision: https://reviews.llvm.org/D115961
Added:
Modified:
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
llvm/test/Transforms/FunctionAttrs/readattrs.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index 17c2b823b035b..70017cb22c2e1 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -681,34 +681,39 @@ determinePointerAccessAttrs(Argument *A,
case Instruction::Call:
case Instruction::Invoke: {
- bool Captures = true;
+ CallBase &CB = cast<CallBase>(*I);
+ if (CB.isCallee(U)) {
+ IsRead = true;
+ // Note that indirect calls do not capture, see comment in
+ // CaptureTracking for context
+ continue;
+ }
- if (I->getType()->isVoidTy())
- Captures = false;
+ // Given we've explictily handled the callee operand above, what's left
+ // must be a data operand (e.g. argument or operand bundle)
+ const unsigned UseIndex = CB.getDataOperandNo(U);
- auto AddUsersToWorklistIfCapturing = [&] {
- if (Captures)
+ if (!CB.doesNotCapture(UseIndex)) {
+ if (!CB.onlyReadsMemory())
+ // If the callee can save a copy into other memory, then simply
+ // scanning uses of the call is insufficient. We have no way
+ // of tracking copies of the pointer through memory to see
+ // if a reloaded copy is written to, thus we must give up.
+ return Attribute::None;
+ // Push users for processing once we finish this one
+ if (!I->getType()->isVoidTy())
for (Use &UU : I->uses())
if (Visited.insert(&UU).second)
Worklist.push_back(&UU);
- };
-
- CallBase &CB = cast<CallBase>(*I);
- if (CB.isCallee(U)) {
- IsRead = true;
- Captures = false; // See comment in CaptureTracking for context
- continue;
}
- if (CB.doesNotAccessMemory()) {
- AddUsersToWorklistIfCapturing();
+
+ if (CB.doesNotAccessMemory())
continue;
- }
Function *F = CB.getCalledFunction();
if (!F) {
if (CB.onlyReadsMemory()) {
IsRead = true;
- AddUsersToWorklistIfCapturing();
continue;
}
return Attribute::None;
@@ -716,21 +721,17 @@ determinePointerAccessAttrs(Argument *A,
// Given we've explictily handled the callee operand above, what's left
// must be a data operand (e.g. argument or operand bundle)
- const unsigned UseIndex = CB.getDataOperandNo(U);
const 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);
if (CB.isArgOperand(U) && SCCNodes.count(F->getArg(UseIndex))) {
// This is an argument which is part of the speculative SCC. Note that
// only operands corresponding to formal arguments of the callee can
// participate in the speculation.
- AddUsersToWorklistIfCapturing();
break;
}
@@ -740,7 +741,6 @@ determinePointerAccessAttrs(Argument *A,
return Attribute::None;
if (!CB.doesNotAccessMemory(UseIndex))
IsRead = true;
- AddUsersToWorklistIfCapturing();
break;
}
@@ -995,6 +995,13 @@ static void addArgumentAttrs(const SCCNodeSet &SCCNodes,
A->addAttr(Attribute::NoCapture);
++NumNoCapture;
Changed.insert(A->getParent());
+
+ // Infer the access attributes given the new nocapture one
+ SmallPtrSet<Argument *, 8> Self;
+ Self.insert(&*A);
+ Attribute::AttrKind R = determinePointerAccessAttrs(&*A, Self);
+ if (R != Attribute::None)
+ addAccessAttr(A, R);
}
continue;
}
diff --git a/llvm/test/Transforms/FunctionAttrs/readattrs.ll b/llvm/test/Transforms/FunctionAttrs/readattrs.ll
index 59f58bdee9aa1..3ca0bc4382277 100644
--- a/llvm/test/Transforms/FunctionAttrs/readattrs.ll
+++ b/llvm/test/Transforms/FunctionAttrs/readattrs.ll
@@ -3,11 +3,9 @@
@x = global i32 0
-declare void @test1_1(i8* %x1_1, i8* readonly %y1_1, ...)
+declare void @test1_1(i8* %x1_1, i8* nocapture readonly %y1_1, ...)
-; NOTE: readonly for %y1_2 would be OK here but not for the similar situation in test13.
-;
-; CHECK: define void @test1_2(i8* %x1_2, i8* readonly %y1_2, i8* %z1_2)
+; CHECK: define void @test1_2(i8* %x1_2, i8* nocapture readonly %y1_2, i8* %z1_2)
define void @test1_2(i8* %x1_2, i8* %y1_2, i8* %z1_2) {
call void (i8*, i8*, ...) @test1_1(i8* %x1_2, i8* %y1_2, i8* %z1_2)
store i32 0, i32* @x
@@ -130,10 +128,8 @@ declare void @escape_readonly_ptr(i8** %addr, i8* readonly %ptr)
; is marked as readnone/only. However, the functions can write the pointer into
; %addr, causing the store to write to %escaped_then_written.
;
-; FIXME: This test currently exposes a bug in function-attrs!
-;
-; CHECK: define void @unsound_readnone(i8* nocapture readnone %ignored, i8* readnone %escaped_then_written)
-; CHECK: define void @unsound_readonly(i8* nocapture readnone %ignored, i8* readonly %escaped_then_written)
+; CHECK: define void @unsound_readnone(i8* nocapture readnone %ignored, i8* %escaped_then_written)
+; CHECK: define void @unsound_readonly(i8* nocapture readnone %ignored, i8* %escaped_then_written)
;
define void @unsound_readnone(i8* %ignored, i8* %escaped_then_written) {
%addr = alloca i8*
More information about the llvm-commits
mailing list