[SimplifyCFG][JumpThreading] Don't duplicate inline-asm instructions #71571
Conversation
…during Jump Threading Conservatively, don't duplicate an inline-asm instruction during Jump Threading. In this particular case, a label is defined in the inline-asm instruction. Duplicating that inline-asm instruction will result in 2 identical labels being defined, which causes an assembler error.
mcinally
requested review from
sanjoy,
zmodem,
yxsamliu,
UsmanNadeem and
xortator
|
@llvm/pr-subscribers-llvm-transforms Author: Cameron McInally (mcinally) ChangesConservatively, don't duplicate an inline-asm instruction during Threading. In this particular case, a label is defined in the inline-asm instruction. Duplicating that inline-asm instruction will result in 2 identical labels being defined, which causes an assembler error. Full diff: https://github.com/llvm/llvm-project/pull/71571.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
index 7a8128c5b6c0901..18d5b7b45ad2618 100644
--- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
@@ -491,8 +491,10 @@ static unsigned getJumpThreadDuplicationCost(const TargetTransformInfo *TTI,
// Blocks with NoDuplicate are modelled as having infinite cost, so they
// are never duplicated.
+ // Conservatively disallow inline-asm instructions. Duplicating inline-asm
+ // instructions can potentially create duplicate labels.
if (const CallInst *CI = dyn_cast<CallInst>(I))
- if (CI->cannotDuplicate() || CI->isConvergent())
+ if (CI->cannotDuplicate() || CI->isConvergent() || CI->isInlineAsm())
return ~0U;
if (TTI->getInstructionCost(&*I, TargetTransformInfo::TCK_SizeAndLatency) ==
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 5ef3a5292af545c..b5ddc2d54fb6a2b 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -3170,9 +3170,10 @@ static bool BlockIsSimpleEnoughToThreadThrough(BasicBlock *BB) {
// Walk the loop in reverse so that we can identify ephemeral values properly
// (values only feeding assumes).
for (Instruction &I : reverse(BB->instructionsWithoutDebug(false))) {
- // Can't fold blocks that contain noduplicate or convergent calls.
+ // Can't fold blocks that contain noduplicate, convergent calls, or
+ // inline assembly.
if (CallInst *CI = dyn_cast<CallInst>(&I))
- if (CI->cannotDuplicate() || CI->isConvergent())
+ if (CI->cannotDuplicate() || CI->isConvergent() || CI->isInlineAsm())
return false;
// Ignore ephemeral values which are deleted during codegen.
diff --git a/llvm/test/Transforms/JumpThreading/inline-asm.ll b/llvm/test/Transforms/JumpThreading/inline-asm.ll
new file mode 100644
index 000000000000000..2e698e934547807
--- /dev/null
+++ b/llvm/test/Transforms/JumpThreading/inline-asm.ll
@@ -0,0 +1,47 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
+; RUN: opt -S -passes=jump-threading < %s | FileCheck %s
+
+; Jump threading should not be able to duplicate a block containing inline
+; assembly, since we do not know what is in the inline-asm instruction.
+
+define void @foo(i32 %n.arg, ptr %b.arg, ptr %c.arg) {
+; CHECK-LABEL: define void @foo(
+; CHECK-SAME: i32 [[N_ARG:%.*]], ptr [[B_ARG:%.*]], ptr [[C_ARG:%.*]]) {
+; CHECK-NEXT: L.entry:
+; CHECK-NEXT: [[TMP0:%.*]] = icmp eq i32 [[N_ARG]], 0
+; CHECK-NEXT: br i1 [[TMP0]], label [[L_B0000:%.*]], label [[L_B0002:%.*]]
+; CHECK: L.B0002:
+; CHECK-NEXT: store ptr null, ptr [[C_ARG]], align 8
+; CHECK-NEXT: store ptr null, ptr [[B_ARG]], align 8
+; CHECK-NEXT: br label [[L_B0000]]
+; CHECK: L.B0000:
+; CHECK-NEXT: call void asm sideeffect " .LABEL: \0A\09", ""()
+; CHECK-NEXT: [[TMP1:%.*]] = icmp eq i32 [[N_ARG]], 0
+; CHECK-NEXT: br i1 [[TMP1]], label [[L_B0001:%.*]], label [[L_B0003:%.*]]
+; CHECK: L.B0003:
+; CHECK-NEXT: [[TMP2:%.*]] = load ptr, ptr [[B_ARG]], align 8
+; CHECK-NEXT: call void @bar(ptr [[TMP2]])
+; CHECK-NEXT: br label [[L_B0001]]
+; CHECK: L.B0001:
+; CHECK-NEXT: ret void
+;
+L.entry:
+ %0 = icmp eq i32 %n.arg, 0
+ br i1 %0, label %L.B0000, label %L.B0002
+L.B0002:
+ store ptr null, ptr %c.arg
+ store ptr null, ptr %b.arg
+ br label %L.B0000
+L.B0000:
+ call void asm sideeffect " .LABEL: \0A\09", "" ()
+ %1 = icmp eq i32 %n.arg, 0
+ br i1 %1, label %L.B0001, label %L.B0003
+L.B0003:
+ %2 = load ptr, ptr %b.arg
+ call void @bar (ptr %2)
+ br label %L.B0001
+L.B0001:
+ ret void
+}
+
+declare void @bar(ptr)
diff --git a/llvm/test/Transforms/SimplifyCFG/inline-asm-threading.ll b/llvm/test/Transforms/SimplifyCFG/inline-asm-threading.ll
new file mode 100644
index 000000000000000..7ea7f373b3d9f88
--- /dev/null
+++ b/llvm/test/Transforms/SimplifyCFG/inline-asm-threading.ll
@@ -0,0 +1,49 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
+; RUN: opt < %s -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S | FileCheck %s
+
+; Jump threading should not be able to duplicate a block containing inline
+; assembly, since we do not know what is in the inline-asm instruction.
+
+define void @foo(i32 %n.arg, ptr %b.arg, ptr %c.arg) {
+; CHECK-LABEL: define void @foo(
+; CHECK-SAME: i32 [[N_ARG:%.*]], ptr [[B_ARG:%.*]], ptr [[C_ARG:%.*]]) {
+; CHECK-NEXT: L.entry:
+; CHECK-NEXT: [[TMP0:%.*]] = icmp eq i32 [[N_ARG]], 0
+; CHECK-NEXT: br i1 [[TMP0]], label [[L_B0000:%.*]], label [[L_B0002:%.*]]
+; CHECK: L.B0002:
+; CHECK-NEXT: store ptr null, ptr [[C_ARG]], align 8
+; CHECK-NEXT: store ptr null, ptr [[B_ARG]], align 8
+; CHECK-NEXT: br label [[L_B0000]]
+; CHECK: L.B0000:
+; CHECK-NEXT: call void asm sideeffect " .LABEL: \0A\09", ""()
+; CHECK-NEXT: br i1 [[TMP0]], label [[L_B0001:%.*]], label [[L_B0003:%.*]]
+; CHECK: L.B0003:
+; CHECK-NEXT: [[TMP1:%.*]] = load ptr, ptr [[B_ARG]], align 8
+; CHECK-NEXT: call void @bar(ptr [[TMP1]])
+; CHECK-NEXT: br label [[L_B0001]]
+; CHECK: L.B0001:
+; CHECK-NEXT: ret void
+;
+L.entry:
+ %0 = icmp eq i32 %n.arg, 0
+ br i1 %0, label %L.B0000, label %L.B0002
+
+L.B0002: ; preds = %L.entry
+ store ptr null, ptr %c.arg
+ store ptr null, ptr %b.arg
+ br label %L.B0000
+
+L.B0000: ; preds = %L.B0002, %L.entry
+ call void asm sideeffect " .LABEL: \0A\09", ""()
+ br i1 %0, label %L.B0001, label %L.B0003
+
+L.B0003: ; preds = %L.B0000
+ %1 = load ptr, ptr %b.arg
+ call void @bar(ptr %1)
+ br label %L.B0001
+
+L.B0001: ; preds = %L.B0003, %L.B0000
+ ret void
+}
+
+declare void @bar(ptr)
|
|
A concern is that this may cause perf degradation for inline asm that allows duplication. Does |
| if (const CallInst *CI = dyn_cast<CallInst>(I)) | ||
| if (CI->cannotDuplicate() || CI->isConvergent()) | ||
| if (CI->cannotDuplicate() || CI->isConvergent() || CI->isInlineAsm()) |
There was a problem hiding this comment.
If we want to say that inline asm cannot be duplicated, then that should be part of cannotDuplicate(), not a separate query. There are a number of other places checking cannotDuplicate().
There was a problem hiding this comment.
We've seen this in the Linux kernel before, pretty sure \@ should be used on the label to make it unique per instantiation. Let me see if I can find the kernel commit I'm thinking of.
(Cant find the kernel commit I'm thinking of). Sorry %= is the sigil. This problem is why it exists.
Under certain circumstances, GCC may duplicate (or remove duplicates of) your assembly code when optimizing. This can lead to unexpected duplicate symbol errors during compilation if your asm code defines symbols or labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
\@ is for asm .macro expansion, but same idea.
https://ftp.gnu.org/old-gnu/Manuals/gas-2.9.1/html_chapter/as_7.html#SEC109
Ah, that's compelling. Thanks, Nick! Closing this issue... |
Conservatively, don't duplicate an inline-asm instruction during Threading. In this particular case, a label is defined in the inline-asm instruction. Duplicating that inline-asm instruction will result in 2 identical labels being defined, which causes an assembler error.