[SimplifyCFG][swifterror] Don't sink calls with swifterror params

[ellishg]

We've encountered an LLVM verification failure when building Swift with the SimplifyCFG pass enabled. I found that https://reviews.llvm.org/D158083 fixed this pass by preventing sinking loads or stores of swifterror values, but it did not implement the same protection for call or invokes.
In Verifier.cpp here and here we can see that swifterror values must also be used directly by call instructions.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Ellis Hoag (ellishg)

Changes

We've encountered an LLVM verification failure when building Swift with the SimplifyCFG pass enabled. I found that https://reviews.llvm.org/D158083 fixed this pass by preventing sinking loads or stores of swifterror values, but it did not implement the same protection for call or invokes.
In Verifier.cpp here and here we can see that swifterror values must also be used directly by call instructions.

---

Full diff: https://github.com/llvm/llvm-project/pull/139015.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/SimplifyCFG.cpp (+8-3)
• (modified) llvm/test/Transforms/SimplifyCFG/hoist-sink-swifterror-store.ll (+53)

diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 33af582a9e0a9..98ecfeb101cd4 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -2190,13 +2190,18 @@ static bool canSinkInstructions(
     if (!I->isSameOperationAs(I0, Instruction::CompareUsingIntersectedAttrs))
       return false;
 
-    // swifterror pointers can only be used by a load or store; sinking a load
-    // or store would require introducing a select for the pointer operand,
-    // which isn't allowed for swifterror pointers.
+    // swifterror pointers can only be used by a load, store, or as a swifterror
+    // argument; sinking a load, store, or call would require introducing a
+    // select for the pointer operand, which isn't allowed for swifterror
+    // pointers.
     if (isa<StoreInst>(I) && I->getOperand(1)->isSwiftError())
       return false;
     if (isa<LoadInst>(I) && I->getOperand(0)->isSwiftError())
       return false;
+    if (const auto *CB = dyn_cast<CallBase>(I))
+      for (const Use &Arg : CB->args())
+        if (Arg->isSwiftError())
+          return false;
 
     // Treat MMRAs conservatively. This pass can be quite aggressive and
     // could drop a lot of MMRAs otherwise.
diff --git a/llvm/test/Transforms/SimplifyCFG/hoist-sink-swifterror-store.ll b/llvm/test/Transforms/SimplifyCFG/hoist-sink-swifterror-store.ll
index 0c13f5703447a..75fb986b20a93 100644
--- a/llvm/test/Transforms/SimplifyCFG/hoist-sink-swifterror-store.ll
+++ b/llvm/test/Transforms/SimplifyCFG/hoist-sink-swifterror-store.ll
@@ -3,6 +3,7 @@
 
 declare void @clobber1()
 declare void @clobber2()
+declare swiftcc void @swift_willThrow(ptr swifterror)
 
 ; Do not try to sink the stores to the exit block, as this requires introducing
 ; a select for the pointer operand. This is not allowed for swifterror pointers.
@@ -76,6 +77,22 @@ exit:
 ; introduces a select for the pointer operand. This is not allowed for
 ; swifterror pointers.
 define swiftcc ptr @sink_load(ptr %arg, ptr swifterror %arg1, i1 %c) {
+; CHECK-LABEL: define swiftcc ptr @sink_load
+; CHECK-SAME: (ptr [[ARG:%.*]], ptr swifterror [[ARG1:%.*]], i1 [[C:%.*]]) {
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    br i1 [[C]], label [[THEN:%.*]], label [[ELSE:%.*]]
+; CHECK:       then:
+; CHECK-NEXT:    call void @clobber1()
+; CHECK-NEXT:    [[L1:%.*]] = load ptr, ptr [[ARG]], align 8
+; CHECK-NEXT:    br label [[EXIT:%.*]]
+; CHECK:       else:
+; CHECK-NEXT:    call void @clobber2()
+; CHECK-NEXT:    [[L2:%.*]] = load ptr, ptr [[ARG1]], align 8
+; CHECK-NEXT:    br label [[EXIT]]
+; CHECK:       exit:
+; CHECK-NEXT:    [[P:%.*]] = phi ptr [ [[L1]], [[THEN]] ], [ [[L2]], [[ELSE]] ]
+; CHECK-NEXT:    ret ptr [[P]]
+;
 bb:
   br i1 %c, label %then, label %else
 
@@ -127,3 +144,39 @@ exit:
   %p = phi ptr [ %l1, %then ], [ %l2, %else ]
   ret ptr %p
 }
+
+
+define swiftcc void @sink_call(i1 %c) {
+; CHECK-LABEL: define swiftcc void @sink_call
+; CHECK-SAME: (i1 [[C:%.*]]) {
+; CHECK-NEXT:    [[TMP1:%.*]] = alloca swifterror ptr, align 8
+; CHECK-NEXT:    [[TMP2:%.*]] = alloca swifterror ptr, align 8
+; CHECK-NEXT:    br i1 [[C]], label [[THEN:%.*]], label [[ELSE:%.*]]
+; CHECK:       then:
+; CHECK-NEXT:    call void @clobber1()
+; CHECK-NEXT:    call swiftcc void @swift_willThrow(ptr [[TMP2]])
+; CHECK-NEXT:    br label [[EXIT:%.*]]
+; CHECK:       else:
+; CHECK-NEXT:    call void @clobber2()
+; CHECK-NEXT:    call swiftcc void @swift_willThrow(ptr [[TMP1]])
+; CHECK-NEXT:    br label [[EXIT]]
+; CHECK:       exit:
+; CHECK-NEXT:    ret void
+;
+  %2 = alloca swifterror ptr, align 8
+  %3 = alloca swifterror ptr, align 8
+  br i1 %c, label %then, label %else
+
+then:
+  call void @clobber1()
+  call swiftcc void @swift_willThrow(ptr %3)
+  br label %exit
+
+else:
+  call void @clobber2()
+  call swiftcc void @swift_willThrow(ptr %2)
+  br label %exit
+
+exit:
+  ret void
+}

[nikic]

I think it would be better to check this in canReplaceOperandWithVariable (and move the existing checks there as well).

In particular, legality here depends on whether sinking requires the introduction of a phi node or not. Could you add test variant where the swifterror argument is the same in both calls, and another argument differs?

[ellishg]

In particular, legality here depends on whether sinking requires the introduction of a phi node or not. Could you add test variant where the swifterror argument is the same in both calls, and another argument differs?

Just to make sure I understand. In this case the sink would be safe, right?

[ellishg]

Let me know if the comment make sense in the context of canReplaceOperandWithVariable()

[efriedma-quic]

I think it might make sense to try to leverage the similarity between swifterror values and the token type. The restrictions are basically the same; it can only be used by a specific set of instructions, and you aren't allowed to obscure a swifterror value. And if the checks are unified, we never need to think about the semantics; you just always check for tokens and swifterror together.

[nikic]

I think it would be better to omit these checks and just always check for isSwiftError().

[ellishg]

I think it might make sense to try to leverage the similarity between swifterror values and the token type. The restrictions are basically the same; it can only be used by a specific set of instructions, and you aren't allowed to obscure a swifterror value. And if the checks are unified, we never need to think about the semantics; you just always check for tokens and swifterror together.

I see that isTokenTy() is used in SimplifyCFG.cpp in a few places. I agree it would be nice to unify token and swifterror, but that might require quite a bit of refactoring and risks breaking token. I would rather leave this refactor to another PR since this PR is trying to fix a correctness bug.

[ellishg]

Friendly ping

[efriedma-quic]

LGTM