[llvm] 452279e - [InstCombine] prevent miscompiles from select-of-div/rem transform
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 1 05:55:57 PST 2023
Author: Sanjay Patel
Date: 2023-03-01T08:54:23-05:00
New Revision: 452279efe21a61c7e17189da99347f7755f27cb0
URL: https://github.com/llvm/llvm-project/commit/452279efe21a61c7e17189da99347f7755f27cb0
DIFF: https://github.com/llvm/llvm-project/commit/452279efe21a61c7e17189da99347f7755f27cb0.diff
LOG: [InstCombine] prevent miscompiles from select-of-div/rem transform
This avoids the danger shown in issue #60906.
There were no regression tests for these patterns, so these potential
failures have been around for a long time.
We freeze the condition and preserve the optimization because
getting rid of a div/rem is always a win.
Here are a couple of examples that can be corrected by freezing the
condition:
https://alive2.llvm.org/ce/z/sXHTTC
Differential Revision: https://reviews.llvm.org/D144671
Added:
Modified:
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
llvm/test/Transforms/InstCombine/select-divrem.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index d4b4a6de1256..e5916981fc3e 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -429,6 +429,21 @@ Instruction *InstCombinerImpl::foldSelectOpOp(SelectInst &SI, Instruction *TI,
!OtherOpF->getType()->isVectorTy()))
return nullptr;
+ // If we are sinking div/rem after a select, we may need to freeze the
+ // condition because div/rem may induce immediate UB with a poison operand.
+ // For example, the following transform is not safe if Cond can ever be poison
+ // because we can replace poison with zero and then we have div-by-zero that
+ // didn't exist in the original code:
+ // Cond ? x/y : x/z --> x / (Cond ? y : z)
+ auto *BO = dyn_cast<BinaryOperator>(TI);
+ if (BO && BO->isIntDivRem() && !isGuaranteedNotToBePoison(Cond)) {
+ // A udiv/urem with a common divisor is safe because UB can only occur with
+ // div-by-zero, and that would be present in the original code.
+ if (BO->getOpcode() == Instruction::SDiv ||
+ BO->getOpcode() == Instruction::SRem || MatchIsOpZero)
+ Cond = Builder.CreateFreeze(Cond);
+ }
+
// If we reach here, they do have operations in common.
Value *NewSI = Builder.CreateSelect(Cond, OtherOpT, OtherOpF,
SI.getName() + ".v", &SI);
diff --git a/llvm/test/Transforms/InstCombine/select-divrem.ll b/llvm/test/Transforms/InstCombine/select-divrem.ll
index 0c81161c9cbc..90e9919f31da 100644
--- a/llvm/test/Transforms/InstCombine/select-divrem.ll
+++ b/llvm/test/Transforms/InstCombine/select-divrem.ll
@@ -1,9 +1,14 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+; It is a miscompile in most of these tests if we
+; execute div/rem without freezing the potentially
+; poison condition value.
+
define i5 @sdiv_common_divisor(i1 %b, i5 %x, i5 %y, i5 %z) {
; CHECK-LABEL: @sdiv_common_divisor(
-; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[B:%.*]], i5 [[Z:%.*]], i5 [[Y:%.*]]
+; CHECK-NEXT: [[TMP1:%.*]] = freeze i1 [[B:%.*]]
+; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[TMP1]], i5 [[Z:%.*]], i5 [[Y:%.*]]
; CHECK-NEXT: [[SEL:%.*]] = sdiv i5 [[SEL_V]], [[X:%.*]]
; CHECK-NEXT: ret i5 [[SEL]]
;
@@ -15,7 +20,8 @@ define i5 @sdiv_common_divisor(i1 %b, i5 %x, i5 %y, i5 %z) {
define i5 @srem_common_divisor(i1 %b, i5 %x, i5 %y, i5 %z) {
; CHECK-LABEL: @srem_common_divisor(
-; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[B:%.*]], i5 [[Z:%.*]], i5 [[Y:%.*]]
+; CHECK-NEXT: [[TMP1:%.*]] = freeze i1 [[B:%.*]]
+; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[TMP1]], i5 [[Z:%.*]], i5 [[Y:%.*]]
; CHECK-NEXT: [[SEL:%.*]] = srem i5 [[SEL_V]], [[X:%.*]]
; CHECK-NEXT: ret i5 [[SEL]]
;
@@ -25,6 +31,9 @@ define i5 @srem_common_divisor(i1 %b, i5 %x, i5 %y, i5 %z) {
ret i5 %sel
}
+; This is ok without freeze because UB can only happen with x==0,
+; and that occurs in the original code.
+
define i5 @udiv_common_divisor(i1 %b, i5 %x, i5 %y, i5 %z) {
; CHECK-LABEL: @udiv_common_divisor(
; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[B:%.*]], i5 [[Z:%.*]], i5 [[Y:%.*]]
@@ -37,6 +46,9 @@ define i5 @udiv_common_divisor(i1 %b, i5 %x, i5 %y, i5 %z) {
ret i5 %sel
}
+; This is ok without freeze because UB can only happen with x==0,
+; and that occurs in the original code.
+
define i5 @urem_common_divisor(i1 %b, i5 %x, i5 %y, i5 %z) {
; CHECK-LABEL: @urem_common_divisor(
; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[B:%.*]], i5 [[Z:%.*]], i5 [[Y:%.*]]
@@ -51,7 +63,8 @@ define i5 @urem_common_divisor(i1 %b, i5 %x, i5 %y, i5 %z) {
define i5 @sdiv_common_dividend(i1 %b, i5 %x, i5 %y, i5 %z) {
; CHECK-LABEL: @sdiv_common_dividend(
-; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[B:%.*]], i5 [[Z:%.*]], i5 [[Y:%.*]]
+; CHECK-NEXT: [[TMP1:%.*]] = freeze i1 [[B:%.*]]
+; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[TMP1]], i5 [[Z:%.*]], i5 [[Y:%.*]]
; CHECK-NEXT: [[SEL:%.*]] = sdiv i5 [[X:%.*]], [[SEL_V]]
; CHECK-NEXT: ret i5 [[SEL]]
;
@@ -63,7 +76,8 @@ define i5 @sdiv_common_dividend(i1 %b, i5 %x, i5 %y, i5 %z) {
define i5 @srem_common_dividend(i1 %b, i5 %x, i5 %y, i5 %z) {
; CHECK-LABEL: @srem_common_dividend(
-; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[B:%.*]], i5 [[Z:%.*]], i5 [[Y:%.*]]
+; CHECK-NEXT: [[TMP1:%.*]] = freeze i1 [[B:%.*]]
+; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[TMP1]], i5 [[Z:%.*]], i5 [[Y:%.*]]
; CHECK-NEXT: [[SEL:%.*]] = srem i5 [[X:%.*]], [[SEL_V]]
; CHECK-NEXT: ret i5 [[SEL]]
;
@@ -75,7 +89,8 @@ define i5 @srem_common_dividend(i1 %b, i5 %x, i5 %y, i5 %z) {
define i5 @udiv_common_dividend(i1 %b, i5 %x, i5 %y, i5 %z) {
; CHECK-LABEL: @udiv_common_dividend(
-; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[B:%.*]], i5 [[Z:%.*]], i5 [[Y:%.*]]
+; CHECK-NEXT: [[TMP1:%.*]] = freeze i1 [[B:%.*]]
+; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[TMP1]], i5 [[Z:%.*]], i5 [[Y:%.*]]
; CHECK-NEXT: [[SEL:%.*]] = udiv i5 [[X:%.*]], [[SEL_V]]
; CHECK-NEXT: ret i5 [[SEL]]
;
@@ -87,7 +102,8 @@ define i5 @udiv_common_dividend(i1 %b, i5 %x, i5 %y, i5 %z) {
define i5 @urem_common_dividend(i1 %b, i5 %x, i5 %y, i5 %z) {
; CHECK-LABEL: @urem_common_dividend(
-; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[B:%.*]], i5 [[Z:%.*]], i5 [[Y:%.*]]
+; CHECK-NEXT: [[TMP1:%.*]] = freeze i1 [[B:%.*]]
+; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[TMP1]], i5 [[Z:%.*]], i5 [[Y:%.*]]
; CHECK-NEXT: [[SEL:%.*]] = urem i5 [[X:%.*]], [[SEL_V]]
; CHECK-NEXT: ret i5 [[SEL]]
;
@@ -97,6 +113,11 @@ define i5 @urem_common_dividend(i1 %b, i5 %x, i5 %y, i5 %z) {
ret i5 %sel
}
+; Repeat the above tests, but guarantee that the select
+; condition is not poison via argument attribute. That
+; makes it safe to execute the select before div/rem
+; without needing to freeze the condition.
+
define i5 @sdiv_common_divisor_defined_cond(i1 noundef %b, i5 %x, i5 %y, i5 %z) {
; CHECK-LABEL: @sdiv_common_divisor_defined_cond(
; CHECK-NEXT: [[SEL_V:%.*]] = select i1 [[B:%.*]], i5 [[Z:%.*]], i5 [[Y:%.*]]
More information about the llvm-commits
mailing list