[SimplifyCFG] Check isSafeToSpeculativelyExecute before computeSpeculationCost

[bcl5980]

Summary

Fix #56038

[nikic]

I believe the real problem here is that canTrap() only checks for division by zero, but not division of SIGNED_MIN by -1, which also traps. This leads to an inconsistency between canTrap() and isSafeToSpeculativelyExecute().

[bcl5980]

In D127950#3588529, @nikic wrote:

I believe the real problem here is that canTrap() only checks for division by zero, but not division of SIGNED_MIN by -1, which also traps. This leads to an inconsistency between canTrap() and isSafeToSpeculativelyExecute().

Is this what you want to do ?

--- a/llvm/lib/IR/Constants.cpp
+++ b/llvm/lib/IR/Constants.cpp
@@ -592,7 +592,10 @@ static bool canTrapImpl(const Constant *C,
   case Instruction::URem:
   case Instruction::SRem:
     // Div and rem can trap if the RHS is not known to be non-zero.
-    if (!isa<ConstantInt>(CE->getOperand(1)) ||CE->getOperand(1)->isNullValue())
+    if (!isa<ConstantInt>(CE->getOperand(1)) ||
+        CE->getOperand(1)->isNullValue() ||
+        (CE->getOperand(0)->isMinSignedValue() &&
+         CE->getOperand(1)->isAllOnesValue()))
       return true;
     return false;
   }

[nikic]

@bcl5980 Conceptually, yes., but I believe your specific patch does not handle sdiv (i64 EXPR, i64 -1) correctly. We also only need this for the signed case.

[bcl5980]

In D127950#3589103, @nikic wrote:

@bcl5980 Conceptually, yes., but I believe your specific patch does not handle sdiv (i64 EXPR, i64 -1) correctly. We also only need this for the signed case.

@nikic, Thanks for the explain. As I have little knowledge for this part can you help to fix the issue? I will abandon this change. Sorry for the disturbance.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

if (canTrap(OrigV) || canTrap(ThenV))
       return false;
 
     HaveRewritablePHIs = true;
     ConstantExpr *OrigCE = dyn_cast<ConstantExpr>(OrigV);
     ConstantExpr *ThenCE = dyn_cast<ConstantExpr>(ThenV);
     if (!OrigCE && !ThenCE)
       continue; // Known cheap (FIXME: Maybe not true for aggregates).
 
-    InstructionCost OrigCost = OrigCE ? computeSpeculationCost(OrigCE, TTI) : 0;
-    InstructionCost ThenCost = ThenCE ? computeSpeculationCost(ThenCE, TTI) : 0;
+    InstructionCost OrigCost = 0;
+    if (OrigCE) {
+      if (!isSafeToSpeculativelyExecute(OrigCE))
+        return false;
+      OrigCost = computeSpeculationCost(OrigCE, TTI);
+    }
+
+    InstructionCost ThenCost = 0;
+    if (ThenCE) {
+      if (!isSafeToSpeculativelyExecute(ThenCE))
+        return false;
+      ThenCost = computeSpeculationCost(ThenCE, TTI);
+    }
+
     InstructionCost MaxCost =
         2 * PHINodeFoldingThreshold * TargetTransformInfo::TCC_Basic;
     if (OrigCost + ThenCost > MaxCost)
       return false;
 
     // Account for the cost of an unfolded ConstantExpr which could end up
     // getting expanded into Instructions.
     // FIXME: This doesn't account for how many operations are combined in the

llvm/test/Transforms/SimplifyCFG/issue-56038.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -simplifycfg < %s | FileCheck %s
+
+@_ZL1e = internal global [2 x i32] zeroinitializer
+@_ZL1d = internal global i32 0
+
+; This test used to crash
+define i32 @test(i1 %b) {
+; CHECK-LABEL: @test(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[COND_I:%.*]] = select i1 [[B:%.*]], i32 0, i32 srem (i32 zext (i1 icmp ne (ptr getelementptr inbounds ([2 x i32], ptr @_ZL1e, i64 0, i64 1), ptr @_ZL1d) to i32), i32 -1)
+; CHECK-NEXT:    ret i32 0
+;
+entry:
+  br i1 %b, label %f.exit, label %cond.false.i
+
+cond.false.i:                                     ; preds = %entry
+  br label %f.exit
+
+f.exit:                                       ; preds = %entry, %cond.false.i
+  %cond.i = phi i32 [ srem (i32 zext (i1 icmp ne (ptr getelementptr inbounds ([2 x i32], ptr @_ZL1e, i64 0, i64 1), ptr @_ZL1d) to i32), i32 -1), %cond.false.i ], [ 0, %entry ]
+  ret i32 0
+}