[llvm] [FastISel][AArch64] Compare Instruction Miscompilation Fix (PR #75993)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 2 09:56:43 PST 2024
https://github.com/brendaso1 updated https://github.com/llvm/llvm-project/pull/75993
>From 56575618c149c4a682be90b5d2fc91604b5179d7 Mon Sep 17 00:00:00 2001
From: Brenda So <mayyeebrenda_so at apple.com>
Date: Tue, 19 Dec 2023 14:44:12 -0800
Subject: [PATCH 1/7] [FastISel][AArch64] compare instruction miscompilation
fix
When shl is folded in compare instruction, a miscompilation occurs
when the CMP instruction is also sign-extended. This fix avoids
folding shl into cmp instruction when the second operand is
extended, and also turns equality operations into unsigned
comparisons.
---
llvm/lib/Target/AArch64/AArch64FastISel.cpp | 8 +++--
.../CodeGen/AArch64/fastisel-shift-and-cmp.ll | 33 +++++++++++++++++++
2 files changed, 39 insertions(+), 2 deletions(-)
create mode 100644 llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
diff --git a/llvm/lib/Target/AArch64/AArch64FastISel.cpp b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
index 9b8162ce8dd4d0..ddd2c4eebe491c 100644
--- a/llvm/lib/Target/AArch64/AArch64FastISel.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
@@ -1233,7 +1233,11 @@ unsigned AArch64FastISel::emitAddSub(bool UseAdd, MVT RetVT, const Value *LHS,
isValueAvailable(RHS)) {
if (const auto *SI = dyn_cast<BinaryOperator>(RHS))
if (const auto *C = dyn_cast<ConstantInt>(SI->getOperand(1)))
- if ((SI->getOpcode() == Instruction::Shl) && (C->getZExtValue() < 4)) {
+ if ((SI->getOpcode() == Instruction::Shl) && (C->getZExtValue() < 4) &&
+ !NeedExtend) {
+ // We can only fold instructions that doesn't need extension because
+ // in add/sub instruction, the instruction is first extended, then
+ // shifted
Register RHSReg = getRegForValue(SI->getOperand(0));
if (!RHSReg)
return 0;
@@ -2426,7 +2430,7 @@ bool AArch64FastISel::selectBranch(const Instruction *I) {
}
// Emit the cmp.
- if (!emitCmp(CI->getOperand(0), CI->getOperand(1), CI->isUnsigned()))
+ if (!emitCmp(CI->getOperand(0), CI->getOperand(1), !CI->isSigned()))
return false;
// FCMP_UEQ and FCMP_ONE cannot be checked with a single branch
diff --git a/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll b/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
new file mode 100644
index 00000000000000..5dad290134bbd7
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
@@ -0,0 +1,33 @@
+; RUN: llc --global-isel=false -fast-isel -O0 -mtriple=aarch64-none-none < %s | FileCheck %s
+
+; Check that the shl instruction did not get folded in together with
+; the cmp instruction. It would create a miscompilation
+
+ at A = dso_local global [5 x i8] c"\C8\AA\C8\AA\AA"
+ at .str = private unnamed_addr constant [13 x i8] c"TRUE BRANCH\0A\00"
+ at .str.1 = private unnamed_addr constant [14 x i8] c"FALSE BRANCH\0A\00"
+
+define dso_local i32 @main() {
+
+ %tmp = load i8, ptr getelementptr inbounds ([5 x i8], ptr @A, i64 0, i64 1)
+ %tmp2 = load i8, ptr getelementptr inbounds ([5 x i8], ptr @A, i64 0, i64 2)
+ %op1 = xor i8 %tmp, -49
+ %op2 = mul i8 %op1, %op1
+; CHECK-NOT: cmp [[REGS:.*]] #[[SHIFT_VAL:[0-9]+]]
+ %op3 = shl i8 %op2, 3
+ %tmp3 = icmp eq i8 %tmp2, %op3
+ br i1 %tmp3, label %_true, label %_false
+
+_true:
+ %res = call i32 (ptr, ...) @printf(ptr noundef @.str)
+ br label %_ret
+
+_false:
+ %res2 = call i32 (ptr, ...) @printf(ptr noundef @.str.1)
+ br label %_ret
+
+_ret:
+ ret i32 0
+}
+
+declare i32 @printf(ptr noundef, ...)
>From cdc10d8cee9744706edab85e84feff2b1133801b Mon Sep 17 00:00:00 2001
From: Brenda So <mayyeebrenda_so at apple.com>
Date: Mon, 25 Dec 2023 16:23:58 +0800
Subject: [PATCH 2/7] Simplify test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
- remove dso_local
- remove the globals
- remove the loads
- remove the calls
- remove --global-isel=false and -O0
---
.../CodeGen/AArch64/fastisel-shift-and-cmp.ll | 32 ++++++-------------
1 file changed, 10 insertions(+), 22 deletions(-)
diff --git a/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll b/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
index 5dad290134bbd7..1067c0428348da 100644
--- a/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
+++ b/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
@@ -1,33 +1,21 @@
-; RUN: llc --global-isel=false -fast-isel -O0 -mtriple=aarch64-none-none < %s | FileCheck %s
+; RUN: llc -fast-isel -mtriple=aarch64-none-none < %s | FileCheck %s
; Check that the shl instruction did not get folded in together with
; the cmp instruction. It would create a miscompilation
- at A = dso_local global [5 x i8] c"\C8\AA\C8\AA\AA"
- at .str = private unnamed_addr constant [13 x i8] c"TRUE BRANCH\0A\00"
- at .str.1 = private unnamed_addr constant [14 x i8] c"FALSE BRANCH\0A\00"
+define i32 @icmp_i8_shift_and_cmp(i8 %a, i8 %b) {
-define dso_local i32 @main() {
-
- %tmp = load i8, ptr getelementptr inbounds ([5 x i8], ptr @A, i64 0, i64 1)
- %tmp2 = load i8, ptr getelementptr inbounds ([5 x i8], ptr @A, i64 0, i64 2)
- %op1 = xor i8 %tmp, -49
+ %op1 = xor i8 %a, -49
%op2 = mul i8 %op1, %op1
; CHECK-NOT: cmp [[REGS:.*]] #[[SHIFT_VAL:[0-9]+]]
%op3 = shl i8 %op2, 3
- %tmp3 = icmp eq i8 %tmp2, %op3
- br i1 %tmp3, label %_true, label %_false
-
-_true:
- %res = call i32 (ptr, ...) @printf(ptr noundef @.str)
- br label %_ret
-
-_false:
- %res2 = call i32 (ptr, ...) @printf(ptr noundef @.str.1)
- br label %_ret
+ %tmp3 = icmp eq i8 %b, %op3
+ %conv = zext i1 %tmp3 to i32
+ ret i32 %conv
+}
-_ret:
- ret i32 0
+define dso_local i32 @main() local_unnamed_addr #0 {
+ %1 = tail call i32 @icmp_i8_shift_and_cmp(i32 noundef 170, i32 noundef 200)
+ ret i32 %1
}
-declare i32 @printf(ptr noundef, ...)
>From 965299998166d20480c3d0d32ed7077de1693530 Mon Sep 17 00:00:00 2001
From: Brenda So <mayyeebrenda_so at apple.com>
Date: Mon, 25 Dec 2023 20:36:30 +0800
Subject: [PATCH 3/7] removed extraneous function in unit test
---
llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll | 5 -----
1 file changed, 5 deletions(-)
diff --git a/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll b/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
index 1067c0428348da..55612e7bfad901 100644
--- a/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
+++ b/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
@@ -14,8 +14,3 @@ define i32 @icmp_i8_shift_and_cmp(i8 %a, i8 %b) {
ret i32 %conv
}
-define dso_local i32 @main() local_unnamed_addr #0 {
- %1 = tail call i32 @icmp_i8_shift_and_cmp(i32 noundef 170, i32 noundef 200)
- ret i32 %1
-}
-
>From 44d4231af845877a3f85fc8488521886e6707af5 Mon Sep 17 00:00:00 2001
From: Brenda So <mayyeebrenda_so at apple.com>
Date: Tue, 26 Dec 2023 22:05:10 +0800
Subject: [PATCH 4/7] clean up unit test
---
llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll | 1 -
1 file changed, 1 deletion(-)
diff --git a/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll b/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
index 55612e7bfad901..b83129513b646e 100644
--- a/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
+++ b/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
@@ -4,7 +4,6 @@
; the cmp instruction. It would create a miscompilation
define i32 @icmp_i8_shift_and_cmp(i8 %a, i8 %b) {
-
%op1 = xor i8 %a, -49
%op2 = mul i8 %op1, %op1
; CHECK-NOT: cmp [[REGS:.*]] #[[SHIFT_VAL:[0-9]+]]
>From a495c27a660cfc42f54cfaf144a0a163f35efa96 Mon Sep 17 00:00:00 2001
From: Brenda So <mayyeebrenda_so at apple.com>
Date: Mon, 1 Jan 2024 19:38:04 +0800
Subject: [PATCH 5/7] Removed left shift and compare merging logic because that
cannot be reached
---
llvm/lib/Target/AArch64/AArch64FastISel.cpp | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/llvm/lib/Target/AArch64/AArch64FastISel.cpp b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
index ddd2c4eebe491c..1c8a3e33b931d3 100644
--- a/llvm/lib/Target/AArch64/AArch64FastISel.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
@@ -1231,19 +1231,6 @@ unsigned AArch64FastISel::emitAddSub(bool UseAdd, MVT RetVT, const Value *LHS,
// Only extend the RHS within the instruction if there is a valid extend type.
if (ExtendType != AArch64_AM::InvalidShiftExtend && RHS->hasOneUse() &&
isValueAvailable(RHS)) {
- if (const auto *SI = dyn_cast<BinaryOperator>(RHS))
- if (const auto *C = dyn_cast<ConstantInt>(SI->getOperand(1)))
- if ((SI->getOpcode() == Instruction::Shl) && (C->getZExtValue() < 4) &&
- !NeedExtend) {
- // We can only fold instructions that doesn't need extension because
- // in add/sub instruction, the instruction is first extended, then
- // shifted
- Register RHSReg = getRegForValue(SI->getOperand(0));
- if (!RHSReg)
- return 0;
- return emitAddSub_rx(UseAdd, RetVT, LHSReg, RHSReg, ExtendType,
- C->getZExtValue(), SetFlags, WantResult);
- }
Register RHSReg = getRegForValue(RHS);
if (!RHSReg)
return 0;
>From 44573157db084ba54545f4cfcc7e987c93ee25cd Mon Sep 17 00:00:00 2001
From: Brenda So <mayyeebrenda_so at apple.com>
Date: Wed, 3 Jan 2024 01:55:39 +0800
Subject: [PATCH 6/7] moved unit test to arm64-fast-isel-icmp.ll
---
.../test/CodeGen/AArch64/arm64-fast-isel-icmp.ll | 16 ++++++++++++++++
.../CodeGen/AArch64/fastisel-shift-and-cmp.ll | 15 ---------------
2 files changed, 16 insertions(+), 15 deletions(-)
delete mode 100644 llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
diff --git a/llvm/test/CodeGen/AArch64/arm64-fast-isel-icmp.ll b/llvm/test/CodeGen/AArch64/arm64-fast-isel-icmp.ll
index f853c0802cb1be..ac08825c237629 100644
--- a/llvm/test/CodeGen/AArch64/arm64-fast-isel-icmp.ll
+++ b/llvm/test/CodeGen/AArch64/arm64-fast-isel-icmp.ll
@@ -1,3 +1,4 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --function icmp_i8_shift_and_cmp --version 4
; RUN: llc -O0 -fast-isel -fast-isel-abort=1 -verify-machineinstrs -mtriple=arm64-apple-darwin < %s | FileCheck %s
define i32 @icmp_eq_imm(i32 %a) nounwind ssp {
@@ -257,3 +258,18 @@ entry:
%conv2 = zext i1 %cmp to i32
ret i32 %conv2
}
+
+define i32 @icmp_i8_shift_and_cmp(i8 %a, i8 %b) {
+entry:
+; CHECK-LABEL: icmp_i8_shift_and_cmp:
+; CHECK: ubfiz [[REG1:w[0-9]+]], w0, #3, #5
+; CHECK-NEXT: sxtb [[REG0:w[0-9]+]], w1
+; CHECK-NEXT: cmp [[REG0]], [[REG1]], sxtb
+; CHECK-NEXT: cset [[REG:w[0-9]+]], eq
+; CHECK-NEXT: and w0, [[REG]], #0x1
+ %op = shl i8 %a, 3
+ %cmp = icmp eq i8 %b, %op
+ %conv = zext i1 %cmp to i32
+ ret i32 %conv
+}
+
diff --git a/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll b/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
deleted file mode 100644
index b83129513b646e..00000000000000
--- a/llvm/test/CodeGen/AArch64/fastisel-shift-and-cmp.ll
+++ /dev/null
@@ -1,15 +0,0 @@
-; RUN: llc -fast-isel -mtriple=aarch64-none-none < %s | FileCheck %s
-
-; Check that the shl instruction did not get folded in together with
-; the cmp instruction. It would create a miscompilation
-
-define i32 @icmp_i8_shift_and_cmp(i8 %a, i8 %b) {
- %op1 = xor i8 %a, -49
- %op2 = mul i8 %op1, %op1
-; CHECK-NOT: cmp [[REGS:.*]] #[[SHIFT_VAL:[0-9]+]]
- %op3 = shl i8 %op2, 3
- %tmp3 = icmp eq i8 %b, %op3
- %conv = zext i1 %tmp3 to i32
- ret i32 %conv
-}
-
>From 7017aec9712ce59678e0bcf14095083175c34df4 Mon Sep 17 00:00:00 2001
From: Brenda So <mayyeebrenda_so at apple.com>
Date: Wed, 3 Jan 2024 01:56:20 +0800
Subject: [PATCH 7/7] removed unnecessary changes to selectBranch
---
llvm/lib/Target/AArch64/AArch64FastISel.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/Target/AArch64/AArch64FastISel.cpp b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
index 1c8a3e33b931d3..1ea63a5d6ec08d 100644
--- a/llvm/lib/Target/AArch64/AArch64FastISel.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
@@ -2417,7 +2417,7 @@ bool AArch64FastISel::selectBranch(const Instruction *I) {
}
// Emit the cmp.
- if (!emitCmp(CI->getOperand(0), CI->getOperand(1), !CI->isSigned()))
+ if (!emitCmp(CI->getOperand(0), CI->getOperand(1), CI->isUnsigned()))
return false;
// FCMP_UEQ and FCMP_ONE cannot be checked with a single branch
More information about the llvm-commits
mailing list