Skip to content

[InstCombine] Add cast support in simplifyUsingControlFlow #142263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andjo403
Copy link
Contributor

@andjo403 andjo403 commented May 31, 2025

Common patterns where phi node can be replaced by trunc, zext or sext.

In the case of a cast and invert is needed this now produces 2 instructions but only removes one phi instruction it is possible to avoid but when I looked at the llvm-opt-benchmark results it looked like it made things better more then worse but is a bit hard to say as there is so many changes.

proof: https://alive2.llvm.org/ce/z/Rppn3Y

closes #54561 closes #67842 closes #67843

@llvmbot
Copy link
Member

llvmbot commented May 31, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Andreas Jonson (andjo403)

Changes

Common patterns where phi node can be replaced by trunc, zext or sext.

proof: https://alive2.llvm.org/ce/z/Rppn3Y

closes #54561 #67842 #67843


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

4 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp (+36-10)
  • (modified) llvm/test/DebugInfo/X86/instcombine-fold-cast-into-phi.ll (+2-2)
  • (modified) llvm/test/Transforms/InstCombine/phi-int-users.ll (+1-1)
  • (modified) llvm/test/Transforms/InstCombine/simple_phi_condition.ll (+7-5)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp b/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
index a842a5edcb8a3..a79d34d2a1898 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
@@ -1325,8 +1325,12 @@ static Value *simplifyUsingControlFlow(InstCombiner &Self, PHINode &PN,
     return nullptr;
   }
 
-  if (Cond->getType() != PN.getType())
-    return nullptr;
+  unsigned CondBitWidth = Cond->getType()->getScalarSizeInBits();
+  unsigned PNBitWidth = PN.getType()->getScalarSizeInBits();
+  Instruction::CastOps CastType =
+      CondBitWidth == PNBitWidth  ? Instruction::BitCast
+      : CondBitWidth < PNBitWidth ? Instruction::ZExt
+                                  : Instruction::Trunc;
 
   // Check that edges outgoing from the idom's terminators dominate respective
   // inputs of the Phi.
@@ -1335,6 +1339,20 @@ static Value *simplifyUsingControlFlow(InstCombiner &Self, PHINode &PN,
     auto *Input = cast<ConstantInt>(std::get<0>(Pair));
     BasicBlock *Pred = std::get<1>(Pair);
     auto IsCorrectInput = [&](ConstantInt *Input) {
+      if (CastType != Instruction::BitCast) {
+        APInt InputValue = Input->getValue();
+        if (CastType == Instruction::Trunc)
+          InputValue = InputValue.zext(CondBitWidth);
+        else if (CastType == Instruction::ZExt &&
+                 InputValue.isIntN(CondBitWidth))
+          InputValue = InputValue.trunc(CondBitWidth);
+        else if (InputValue.isSignedIntN(CondBitWidth)) {
+          CastType = Instruction::SExt;
+          InputValue = InputValue.trunc(CondBitWidth);
+        } else
+          return false;
+        Input = ConstantInt::get(Context, InputValue);
+      }
       // The input needs to be dominated by the corresponding edge of the idom.
       // This edge cannot be a multi-edge, as that would imply that multiple
       // different condition values follow the same edge.
@@ -1360,19 +1378,27 @@ static Value *simplifyUsingControlFlow(InstCombiner &Self, PHINode &PN,
     Invert = NeedsInvert;
   }
 
-  if (!*Invert)
+  if (!*Invert && CastType == Instruction::BitCast)
     return Cond;
 
-  // This Phi is actually opposite to branching condition of IDom. We invert
-  // the condition that will potentially open up some opportunities for
-  // sinking.
   auto InsertPt = BB->getFirstInsertionPt();
-  if (InsertPt != BB->end()) {
-    Self.Builder.SetInsertPoint(&*BB, InsertPt);
-    return Self.Builder.CreateNot(Cond);
+  if (InsertPt == BB->end())
+    return nullptr;
+
+  Self.Builder.SetInsertPoint(&*BB, InsertPt);
+
+  if (CastType != Instruction::BitCast) {
+    Cond = Self.Builder.CreateCast(CastType, Cond, PN.getType());
+    if (auto *Trunc = dyn_cast<TruncInst>(Cond))
+      Trunc->setHasNoUnsignedWrap(true);
+    if (!*Invert)
+      return Cond;
   }
 
-  return nullptr;
+  // This Phi is actually opposite to branching condition of IDom. We invert
+  // the condition that will potentially open up some opportunities for
+  // sinking.
+  return Self.Builder.CreateNot(Cond);
 }
 
 // Fold  iv = phi(start, iv.next = iv2.next op start)
diff --git a/llvm/test/DebugInfo/X86/instcombine-fold-cast-into-phi.ll b/llvm/test/DebugInfo/X86/instcombine-fold-cast-into-phi.ll
index a91a983be49bc..3ec226fc58d6b 100644
--- a/llvm/test/DebugInfo/X86/instcombine-fold-cast-into-phi.ll
+++ b/llvm/test/DebugInfo/X86/instcombine-fold-cast-into-phi.ll
@@ -4,7 +4,7 @@
 ;; user (otherwise the dbg use becomes poison after the original phi is
 ;; deleted). Check the new phi inherits the DebugLoc.
 
-; CHECK: %[[phi:.*]] = phi i8 [ 1, %{{.*}} ], [ 0, %{{.*}} ], !dbg ![[dbg:[0-9]+]]
+; CHECK: %[[phi:.*]] = phi i8 [ 2, %{{.*}} ], [ 0, %{{.*}} ], !dbg ![[dbg:[0-9]+]]
 ; CHECK: #dbg_value(i8 %[[phi]], ![[#]], !DIExpression(DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)
 ; CHECK: ![[dbg]] = !DILocation(line: 123,
 
@@ -19,7 +19,7 @@ if.then:                                          ; preds = entry
   br label %if.end
 
 if.end:                                           ; preds = %if.then, %entry
-  %p.0 = phi i32 [ 1, %if.then ], [ 0, %entry ], !dbg !13
+  %p.0 = phi i32 [ 2, %if.then ], [ 0, %entry ], !dbg !13
   call void @llvm.dbg.value(metadata i32 %p.0, metadata !4, metadata !DIExpression()), !dbg !13
   %x = trunc i32 %p.0 to i8
   %callff = call float @ff(i8  %x)
diff --git a/llvm/test/Transforms/InstCombine/phi-int-users.ll b/llvm/test/Transforms/InstCombine/phi-int-users.ll
index 6c98cc8a1c900..cc70feb160a08 100644
--- a/llvm/test/Transforms/InstCombine/phi-int-users.ll
+++ b/llvm/test/Transforms/InstCombine/phi-int-users.ll
@@ -13,7 +13,7 @@ define void @f1(i1 %a) {
 ; CHECK:       [[BB2]]:
 ; CHECK-NEXT:    br label %[[BB3]]
 ; CHECK:       [[BB3]]:
-; CHECK-NEXT:    [[PHI:%.*]] = phi i64 [ 0, %[[BB2]] ], [ 1, %[[BB1]] ]
+; CHECK-NEXT:    [[PHI:%.*]] = zext i1 [[A]] to i64
 ; CHECK-NEXT:    [[INTTOPTR:%.*]] = inttoptr i64 [[PHI]] to ptr
 ; CHECK-NEXT:    store i32 0, ptr [[INTTOPTR]], align 4
 ; CHECK-NEXT:    br label %[[BB1]]
diff --git a/llvm/test/Transforms/InstCombine/simple_phi_condition.ll b/llvm/test/Transforms/InstCombine/simple_phi_condition.ll
index 544ebaecaa752..5e299e945c51d 100644
--- a/llvm/test/Transforms/InstCombine/simple_phi_condition.ll
+++ b/llvm/test/Transforms/InstCombine/simple_phi_condition.ll
@@ -669,7 +669,7 @@ define i32 @test_phi_to_zext(i8 noundef %0) {
 ; CHECK:       bb1:
 ; CHECK-NEXT:    br label [[BB5]]
 ; CHECK:       bb5:
-; CHECK-NEXT:    [[DOT0:%.*]] = phi i32 [ 1, [[BB1]] ], [ 0, [[BB4]] ], [ 255, [[BB3]] ]
+; CHECK-NEXT:    [[DOT0:%.*]] = zext i8 [[TMP0]] to i32
 ; CHECK-NEXT:    ret i32 [[DOT0]]
 ;
 start:
@@ -713,7 +713,7 @@ define i32 @test_phi_to_sext(i8 noundef %0) {
 ; CHECK:       bb1:
 ; CHECK-NEXT:    br label [[BB5]]
 ; CHECK:       bb5:
-; CHECK-NEXT:    [[DOT0:%.*]] = phi i32 [ 1, [[BB1]] ], [ 0, [[BB4]] ], [ -1, [[BB3]] ]
+; CHECK-NEXT:    [[DOT0:%.*]] = sext i8 [[TMP0]] to i32
 ; CHECK-NEXT:    ret i32 [[DOT0]]
 ;
 start:
@@ -757,7 +757,8 @@ define i32 @test_phi_to_sext_inverted(i8 noundef %0) {
 ; CHECK:       bb1:
 ; CHECK-NEXT:    br label [[BB5]]
 ; CHECK:       bb5:
-; CHECK-NEXT:    [[DOT0:%.*]] = phi i32 [ -2, [[BB1]] ], [ -1, [[BB4]] ], [ 0, [[BB3]] ]
+; CHECK-NEXT:    [[TMP1:%.*]] = xor i8 [[TMP0]], -1
+; CHECK-NEXT:    [[DOT0:%.*]] = sext i8 [[TMP1]] to i32
 ; CHECK-NEXT:    ret i32 [[DOT0]]
 ;
 start:
@@ -843,7 +844,7 @@ define i8 @test_phi_to_trunc(i32 %0) {
 ; CHECK:       bb1:
 ; CHECK-NEXT:    br label [[BB5]]
 ; CHECK:       bb5:
-; CHECK-NEXT:    [[DOT0:%.*]] = phi i8 [ -1, [[BB1]] ], [ 1, [[BB4]] ], [ 0, [[START:%.*]] ]
+; CHECK-NEXT:    [[DOT0:%.*]] = trunc nuw i32 [[TMP0]] to i8
 ; CHECK-NEXT:    ret i8 [[DOT0]]
 ;
 start:
@@ -882,7 +883,8 @@ define i8 @test_phi_to_trunc_inverted(i32 %0) {
 ; CHECK:       bb1:
 ; CHECK-NEXT:    br label [[BB5]]
 ; CHECK:       bb5:
-; CHECK-NEXT:    [[DOT0:%.*]] = phi i8 [ 0, [[BB1]] ], [ -2, [[BB4]] ], [ -1, [[START:%.*]] ]
+; CHECK-NEXT:    [[TMP1:%.*]] = trunc nuw i32 [[TMP0]] to i8
+; CHECK-NEXT:    [[DOT0:%.*]] = xor i8 [[TMP1]], -1
 ; CHECK-NEXT:    ret i8 [[DOT0]]
 ;
 start:

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miscompilation reproducer: https://alive2.llvm.org/ce/z/7aGmsm

define i32 @src(i32 %0) {
entry:
  %cmp120.not.i.i = icmp ne i32 %0, 0
  br i1 %cmp120.not.i.i, label %for.cond150.preheader.i.i, label %func_126.exit

for.cond150.preheader.i.i:                        ; preds = %entry
  br label %func_126.exit

func_126.exit:                                    ; preds = %for.cond150.preheader.i.i, %entry
  %storemerge14001549.lcssa.i.i = phi i32 [ -2, %entry ], [ -1, %for.cond150.preheader.i.i ]
  ret i32 %storemerge14001549.lcssa.i.i
}

define i32 @tgt(i32 %0) {
entry:
  %cmp120.not.i.i.not = icmp ne i32 %0, 0
  br i1 %cmp120.not.i.i.not, label %for.cond150.preheader.i.i, label %func_126.exit

for.cond150.preheader.i.i:                        ; preds = %entry
  br label %func_126.exit

func_126.exit:                                    ; preds = %for.cond150.preheader.i.i, %entry
  %storemerge14001549.lcssa.i.i = sext i1 %cmp120.not.i.i.not to i32
  ret i32 %storemerge14001549.lcssa.i.i
}

@andjo403
Copy link
Contributor Author

andjo403 commented Jun 1, 2025

fixed the misscompile CastType was updated to sext but the value was not matched and then the value was inverted and the value matched so the expected zext was changed faulty to a sext.

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miscompilation reproducer: https://alive2.llvm.org/ce/z/jEQKTt

define i32 @src(i32 %0, i32 %1) {
entry:
  %tobool265.not.i.i.i330.i = icmp eq i32 %0, 0
  br i1 %tobool265.not.i.i.i330.i, label %if.else351.i.i.i356.i, label %cleanup748.i.i.i.i

if.else351.i.i.i356.i:                            ; preds = %entry
  %tobool380.not.not.i.i.i357.i = icmp eq i32 %1, 0
  br i1 %tobool380.not.not.i.i.i357.i, label %cleanup748.loopexit.i.i.i.i, label %cleanup748.i.i.i.i

cleanup748.loopexit.i.i.i.i:                      ; preds = %if.else351.i.i.i356.i
  br label %cleanup748.i.i.i.i

cleanup748.i.i.i.i:                               ; preds = %cleanup748.loopexit.i.i.i.i, %if.else351.i.i.i356.i, %entry
  %2 = phi i32 [ 1, %cleanup748.loopexit.i.i.i.i ], [ 0, %entry ], [ -1, %if.else351.i.i.i356.i ]
  ret i32 %2
}

define i32 @tgt(i32 %0, i32 %1) {
entry:
  %tobool265.not.i.i.i330.i = icmp eq i32 %0, 0
  br i1 %tobool265.not.i.i.i330.i, label %if.else351.i.i.i356.i, label %cleanup748.i.i.i.i

if.else351.i.i.i356.i:                            ; preds = %entry
  %tobool380.not.not.i.i.i357.i = icmp eq i32 %1, 0
  br i1 %tobool380.not.not.i.i.i357.i, label %cleanup748.loopexit.i.i.i.i, label %cleanup748.i.i.i.i

cleanup748.loopexit.i.i.i.i:                      ; preds = %if.else351.i.i.i356.i
  br label %cleanup748.i.i.i.i

cleanup748.i.i.i.i:                               ; preds = %cleanup748.loopexit.i.i.i.i, %if.else351.i.i.i356.i, %entry
  %2 = sext i1 %tobool265.not.i.i.i330.i to i32
  ret i32 %2
}

andjo403 added a commit that referenced this pull request Jun 7, 2025
@andjo403 andjo403 force-pushed the simplifyUsingControlFlowDifferentTypes branch from 75d327c to b9cd63e Compare June 7, 2025 10:01
@andjo403
Copy link
Contributor Author

andjo403 commented Jun 7, 2025

was the handling of sext that was still a problem have reworked the solution to have a separat pass for the sext fold

@andjo403
Copy link
Contributor Author

andjo403 commented Jun 7, 2025

also thanks @dtcxzyw for finding the miscompilations

rorth pushed a commit to rorth/llvm-project that referenced this pull request Jun 11, 2025
DhruvSrivastavaX pushed a commit to DhruvSrivastavaX/lldb-for-aix that referenced this pull request Jun 12, 2025
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
@andjo403
Copy link
Contributor Author

Seems to have passed the fuzzing now so think this is ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missed optimization of switch to arithmetic Missed optimization of switch to arithmetic switch not recognized as equivalent to sext
3 participants