[verify][swift] Allow passing swifterror to phi instructions #138598
Conversation
|
@llvm/pr-subscribers-llvm-ir Author: Ellis Hoag (ellishg) ChangesWe encountered some code that failed swift verification because some swifterror values were passed to phi instructions. I believe this should be valid, so this PR adjusts the verification code to allow this. Full diff: https://github.com/llvm/llvm-project/pull/138598.diff 2 Files Affected:
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index a798808d79656..2e5ff4b5df8dd 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -3653,18 +3653,36 @@ void Verifier::visitCallBase(CallBase &Call) {
// well.
for (unsigned i = 0, e = FTy->getNumParams(); i != e; ++i) {
if (Call.paramHasAttr(i, Attribute::SwiftError)) {
- Value *SwiftErrorArg = Call.getArgOperand(i);
- if (auto AI = dyn_cast<AllocaInst>(SwiftErrorArg->stripInBoundsOffsets())) {
- Check(AI->isSwiftError(),
- "swifterror argument for call has mismatched alloca", AI, Call);
- continue;
+ SetVector<const Value *> Args;
+ Args.insert(Call.getArgOperand(i));
+ bool DidChange;
+ do {
+ DidChange = false;
+ // Inherit the incoming values of phi instructions to capture all
+ // values.
+ for (const Value *Arg : Args)
+ if (auto *PhiI = dyn_cast<PHINode>(Arg))
+ for (const auto &Op : PhiI->incoming_values())
+ DidChange |= Args.insert(Op.get());
+ } while (DidChange);
+
+ for (const Value *SwiftErrorArg : Args) {
+ if (isa<PHINode>(SwiftErrorArg))
+ continue;
+ if (auto AI =
+ dyn_cast<AllocaInst>(SwiftErrorArg->stripInBoundsOffsets())) {
+ Check(AI->isSwiftError(),
+ "swifterror argument for call has mismatched alloca", AI, Call);
+ continue;
+ }
+ auto ArgI = dyn_cast<Argument>(SwiftErrorArg);
+ Check(ArgI,
+ "swifterror argument should come from an alloca or parameter",
+ SwiftErrorArg, Call);
+ Check(ArgI->hasSwiftErrorAttr(),
+ "swifterror argument for call has mismatched parameter", ArgI,
+ Call);
}
- auto ArgI = dyn_cast<Argument>(SwiftErrorArg);
- Check(ArgI, "swifterror argument should come from an alloca or parameter",
- SwiftErrorArg, Call);
- Check(ArgI->hasSwiftErrorAttr(),
- "swifterror argument for call has mismatched parameter", ArgI,
- Call);
}
if (Attrs.hasParamAttr(i, Attribute::ImmArg)) {
@@ -4356,11 +4374,23 @@ void Verifier::verifySwiftErrorCall(CallBase &Call,
}
void Verifier::verifySwiftErrorValue(const Value *SwiftErrorVal) {
+ SetVector<const User *> Users(SwiftErrorVal->users().begin(),
+ SwiftErrorVal->users().end());
+ bool DidChange;
+ do {
+ DidChange = false;
+ // Inherit the users of phi instructions to capture all users.
+ for (const User *U : Users)
+ if (auto PhiI = dyn_cast<PHINode>(U))
+ for (const User *U : PhiI->users())
+ DidChange |= Users.insert(U);
+ } while (DidChange);
+
// Check that swifterror value is only used by loads, stores, or as
// a swifterror argument.
- for (const User *U : SwiftErrorVal->users()) {
+ for (const User *U : Users) {
Check(isa<LoadInst>(U) || isa<StoreInst>(U) || isa<CallInst>(U) ||
- isa<InvokeInst>(U),
+ isa<InvokeInst>(U) || isa<PHINode>(U),
"swifterror value can only be loaded and stored from, or "
"as a swifterror argument!",
SwiftErrorVal, U);
diff --git a/llvm/test/Verifier/swifterror.ll b/llvm/test/Verifier/swifterror.ll
index d27b43234cadc..ae340d24068fb 100644
--- a/llvm/test/Verifier/swifterror.ll
+++ b/llvm/test/Verifier/swifterror.ll
@@ -1,4 +1,4 @@
-; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s --implicit-check-not=swifterror
%swift_error = type {i64, i8}
@@ -10,6 +10,36 @@ define float @foo(ptr swifterror %error_ptr_ref) {
ret float 1.0
}
+; CHECK: swifterror value can only be loaded and stored from, or as a swifterror argument!
+; CHECK: %ptr0 = alloca swifterror ptr, align 8
+; CHECK: %t = getelementptr inbounds ptr, ptr %err, i64 1
+; CHECK: swifterror value can only be loaded and stored from, or as a swifterror argument!
+; CHECK: %ptr1 = alloca swifterror ptr, align 8
+; CHECK: %t = getelementptr inbounds ptr, ptr %err, i64 1
+define float @phi(i1 %a) {
+entry:
+ %ptr0 = alloca swifterror ptr, align 8
+ %ptr1 = alloca swifterror ptr, align 8
+ %ptr2 = alloca ptr, align 8
+ br i1 %a, label %body, label %body2
+
+body:
+ %err = phi ptr [ %ptr0, %entry ], [ %ptr1, %body ]
+ %t = getelementptr inbounds ptr, ptr %err, i64 1
+ br label %body
+
+; CHECK: swifterror argument for call has mismatched alloca
+; CHECK: %ptr2 = alloca ptr, align 8
+; CHECK: %call = call float @foo(ptr swifterror %err_bad)
+body2:
+ %err_bad = phi ptr [ %ptr0, %entry ], [ %ptr2, %body2 ]
+ %call = call float @foo(ptr swifterror %err_bad)
+ br label %body2
+
+end:
+ ret float 1.0
+}
+
; CHECK: swifterror argument for call has mismatched alloca
; CHECK: %error_ptr_ref = alloca ptr
; CHECK: %call = call float @foo(ptr swifterror %error_ptr_ref)
@@ -22,12 +52,14 @@ entry:
}
; CHECK: swifterror alloca must have pointer type
+; CHECK: %a = alloca swifterror i128, align 8
define void @swifterror_alloca_invalid_type() {
%a = alloca swifterror i128
ret void
}
; CHECK: swifterror alloca must not be array allocation
+; CHECK: %a = alloca swifterror ptr, i64 2, align 8
define void @swifterror_alloca_array() {
%a = alloca swifterror ptr, i64 2
ret void
|
There was a problem hiding this comment.
This has a quadratic runtime in number of PHIs! Better use a worklist algorithm
SmallVector<Value*> PHIWorkList;
for each call Parameter:
if (swifterror && isPhi) PHIWorkList.push_back(parameter);
while (!PHIWorkList.empty()) {
Value* PHI = PHIWorkList.pop_back();
for each operand in PHI.operands:
Args.insert(opperand);
if (added && isPhi(operand)) {
PHIWorkList.push_back(operand);
}
}
// ...
llvm/lib/IR/Verifier.cpp
Outdated
| const auto *PhiI = PHIWorkList.pop_back_val(); | ||
| for (const auto &Op : PhiI->incoming_values()) { | ||
| const auto *NextPhiI = dyn_cast<PHINode>(Op.get()); | ||
| if (Values.insert(Op.get()) && NextPhiI) | ||
| PHIWorkList.push_back(NextPhiI); | ||
| } |
There was a problem hiding this comment.
Does this work using implicit conversion from Use to Value?
| const auto *PhiI = PHIWorkList.pop_back_val(); | |
| for (const auto &Op : PhiI->incoming_values()) { | |
| const auto *NextPhiI = dyn_cast<PHINode>(Op.get()); | |
| if (Values.insert(Op.get()) && NextPhiI) | |
| PHIWorkList.push_back(NextPhiI); | |
| } | |
| const auto *PhiI = PHIWorkList.pop_back_val(); | |
| for (Value *Op : PhiI->incoming_values()) { | |
| const auto *NextPhiI = dyn_cast<PHINode>(Op); | |
| if (Values.insert(Op) && NextPhiI) | |
| PHIWorkList.push_back(NextPhiI); | |
| } |
There was a problem hiding this comment.
Nice, apparently this works!
|
Any background on why people haven't previously encountered PHI nodes here? |
I'm not sure, but I can shed some light on how I encountered it. I suspect that most people compiling Swift code do not run the verifier. It turns out that |
There was a problem hiding this comment.
If you are allowing phi nodes, you need to allow selects as well. This should also result in a LangRef change.
But this really needs confirmation from someone familiar with swifterror that this is safe. I think that used to be @TNorthover, but I'm not sure who it is now. @fhahn Do you know who should review swifterror stuff?
|
The intend of the To do this, we model the "value" as "in memory" and the later assign a register to it during lowering (that mem2reg used to be done in ISel, these days it is SwiftErrorValueTracking.cpp I think). Having https://llvm.org/docs/LangRef.html#parameter-attributes
|
|
Thanks for the help! I'll try to figure out what's going on |
|
I was able to find a reproducer and just submitted #139015 |
We encountered some code that failed swift verification because some swifterror values were passed to phi instructions. I believe this should be valid, so this PR adjusts the verification code to allow this.