[InstCombine] Simplify multi use cast of Phi with constant inputs

[andjo403]

Regression noticed in #184182

for icmp there is a similar fold here https://github.com/andjo403/llvm-project/blob/393db14ac027412180d94ddcd35fb25b09a45913/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp#L1346-L1361

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Andreas Jonson (andjo403)

Changes

Regression noticed in #184182

for icmp there is a similar fold here https://github.com/andjo403/llvm-project/blob/393db14ac027412180d94ddcd35fb25b09a45913/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp#L1346-L1361

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp (+6-1)
• (modified) llvm/test/Transforms/InstCombine/cast_phi.ll (+100)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
index f2a2e21f6fc95..fcd46b3ba4a17 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
@@ -237,7 +237,12 @@ Instruction *InstCombinerImpl::commonCastTransforms(CastInst &CI) {
     // legal type.
     if (!Src->getType()->isIntegerTy() || !CI.getType()->isIntegerTy() ||
         shouldChangeType(CI.getSrcTy(), CI.getType()))
-      if (Instruction *NV = foldOpIntoPhi(CI, PN))
+      if (Instruction *NV =
+              foldOpIntoPhi(CI, PN,
+                            is_contained({Instruction::Trunc, Instruction::ZExt,
+                                          Instruction::SExt},
+                                         CI.getOpcode()) &&
+                                all_of(PN->operands(), IsaPred<ConstantInt>)))
         return NV;
   }
 
diff --git a/llvm/test/Transforms/InstCombine/cast_phi.ll b/llvm/test/Transforms/InstCombine/cast_phi.ll
index e8db72c8d849e..59929ee420f49 100644
--- a/llvm/test/Transforms/InstCombine/cast_phi.ll
+++ b/llvm/test/Transforms/InstCombine/cast_phi.ll
@@ -378,3 +378,103 @@ exit:
   %ext = zext i8 %iv to i32
   ret i32 %ext
 }
+
+
+declare void @use8(i8)
+declare void @use32(i32)
+
+define i32 @zext_constants_multi_use(i8 %x) {
+; CHECK-LABEL: @zext_constants_multi_use(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8 [[X:%.*]], 42
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       t:
+; CHECK-NEXT:    br label [[EXIT:%.*]]
+; CHECK:       f:
+; CHECK-NEXT:    br label [[EXIT]]
+; CHECK:       exit:
+; CHECK-NEXT:    [[R:%.*]] = phi i32 [ 5, [[T]] ], [ 255, [[F]] ]
+; CHECK-NEXT:    [[P:%.*]] = phi i8 [ 5, [[T]] ], [ -1, [[F]] ]
+; CHECK-NEXT:    call void @use8(i8 [[P]])
+; CHECK-NEXT:    ret i32 [[R]]
+;
+entry:
+  %cmp = icmp eq i8 %x, 42
+  br i1 %cmp, label %t, label %f
+
+t:
+  br label %exit
+
+f:
+  br label %exit
+
+exit:
+  %p = phi i8 [ 5, %t ], [ -1, %f ]
+  call void @use8(i8 %p)
+  %r = zext i8 %p to i32
+  ret i32 %r
+}
+
+define i32 @sext_constants_multi_use(i8 %x) {
+; CHECK-LABEL: @sext_constants_multi_use(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8 [[X:%.*]], 42
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       t:
+; CHECK-NEXT:    br label [[EXIT:%.*]]
+; CHECK:       f:
+; CHECK-NEXT:    br label [[EXIT]]
+; CHECK:       exit:
+; CHECK-NEXT:    [[R:%.*]] = phi i32 [ 5, [[T]] ], [ -1, [[F]] ]
+; CHECK-NEXT:    [[P:%.*]] = phi i8 [ 5, [[T]] ], [ -1, [[F]] ]
+; CHECK-NEXT:    call void @use8(i8 [[P]])
+; CHECK-NEXT:    ret i32 [[R]]
+;
+entry:
+  %cmp = icmp eq i8 %x, 42
+  br i1 %cmp, label %t, label %f
+
+t:
+  br label %exit
+
+f:
+  br label %exit
+
+exit:
+  %p = phi i8 [ 5, %t ], [ -1, %f ]
+  call void @use8(i8 %p)
+  %r = sext i8 %p to i32
+  ret i32 %r
+}
+
+define i8 @trunc_constants_multi_use(i8 %x) {
+; CHECK-LABEL: @trunc_constants_multi_use(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8 [[X:%.*]], 42
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       t:
+; CHECK-NEXT:    br label [[EXIT:%.*]]
+; CHECK:       f:
+; CHECK-NEXT:    br label [[EXIT]]
+; CHECK:       exit:
+; CHECK-NEXT:    [[R:%.*]] = phi i8 [ 5, [[T]] ], [ -1, [[F]] ]
+; CHECK-NEXT:    [[P:%.*]] = phi i32 [ 5, [[T]] ], [ -1, [[F]] ]
+; CHECK-NEXT:    call void @use32(i32 [[P]])
+; CHECK-NEXT:    ret i8 [[R]]
+;
+entry:
+  %cmp = icmp eq i8 %x, 42
+  br i1 %cmp, label %t, label %f
+
+t:
+  br label %exit
+
+f:
+  br label %exit
+
+exit:
+  %p = phi i32 [ 5, %t ], [ -1, %f ]
+  call void @use32(i32 %p)
+  %r = trunc i32 %p to i8
+  ret i8 %r
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[andjo403]

possible to set AllowMultipleUses to true as there is a check in foldOpIntoPhi that only allow multi use of the instruction if all inputs is simplified but maybe wast compile time?

[dtcxzyw]

Sorry, I am too late :( May I ask you what the motivating case is? If the new phi cannot be eliminated by follow-up transformations, the backend has to materialize incoming constants twice :( I don't see the benefit.

https://github.com/andjo403/llvm-project/blob/393db14ac027412180d94ddcd35fb25b09a45913/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp#L1346-L1361 is unavailable now. I guess the motivating case is trunc nuw %phi to i1? If so, can we be more conservative and just match the specific pattern?

[andjo403]

the motivation was:
dtcxzyw/llvm-opt-benchmark#3528 (comment)

And noticed that it was folded when it was a icmp ne %x, 0 in

llvm-project/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

Lines 1347 to 1362 in ec5862a

	if (auto *Phi = dyn_cast<PHINode>(Op0))
	if (all_of(Phi->operands(), IsaPred<Constant>)) {
	SmallVector<Constant *> Ops;
	for (Value *V : Phi->incoming_values()) {
	Constant *Res =
	ConstantFoldCompareInstOperands(Pred, cast<Constant>(V), C, DL);
	if (!Res)
	return nullptr;
	Ops.push_back(Res);
	}
	Builder.SetInsertPoint(Phi);
	PHINode *NewPhi = Builder.CreatePHI(Cmp.getType(), Phi->getNumOperands());
	for (auto [V, Pred] : zip(Ops, Phi->blocks()))
	NewPhi->addIncoming(V, Pred);
	return replaceInstUsesWith(Cmp, NewPhi);
	}

but yes probably makes more sense only for trunc nuw but will still be a bit of gamble that something simplifies to remove the extra phi