[PowerPC] PPCBoolRetToInt: Don't translate Constant's operands

[lkail]

Summary

When collecting i1 values via findAllDefs, ignore Constant's operands, since Constant's operands might not be i1.

Fixes https://bugs.llvm.org/show_bug.cgi?id=46923 which causes ICE

llvm-project/llvm/lib/IR/Constants.cpp:1924: static llvm::Constant *llvm::ConstantExpr::getZExt(llvm::Constant *, llvm::Type *, bool): Assertion `C->getType()->getScalarSizeInBits() < Ty->getScalarSizeInBits()&& "SrcTy must be smaller than DestTy for ZExt!"' failed.

[tstellar]

I can confirm that this patch also fixes the full test case.

[lkail]

Thanks Tom for testing it. Gentle ping reviewers.

[hans]

Carrot, Kit: I think you're most familiar with this code. Ping?

[lkail]

Ping

[Carrot]

I think your test case is a perfect candidate for this optimization. It should not be skipped.

The real problem is in function findAllDefs. It should not find out the operands of Constant and push them into Defs.
Suppose we have following ConstantExpr C0 as a phi operand

C0 = C1 > C2

C0 is i1 type, C1 and C2 can be i64 type. And later we should call function translate on C0 only. Translate the type of C1 and C2 doesn't make sense.

[lkail]

@Carrot Thanks for your awesome solution.

[Carrot]

Please also update your patch description. Otherwise it looks good to me.

[lkail]

Updated Summary.

[Carrot]

LGTM

[hans]

Pushed to 11.x as b931e22c954374acf75c4f1d1f2666f3f8e67470. Please let me know if there are any follow-ups.

llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp

static SmallPtrSet<Value *, 8> findAllDefs(Value *V) {
     SmallPtrSet<Value *, 8> Defs;
     SmallVector<Value *, 8> WorkList;
     WorkList.push_back(V);
     Defs.insert(V);
     while (!WorkList.empty()) {
       Value *Curr = WorkList.back();
       WorkList.pop_back();
       auto *CurrUser = dyn_cast<User>(Curr);
-      // Operands of CallInst are skipped because they may not be Bool type,
-      // and their positions are defined by ABI.
-      if (CurrUser && !isa<CallInst>(Curr))
+      // Operands of CallInst/Constant are skipped because they may not be Bool
+      // type. For CallInst, their positions are defined by ABI.
+      if (CurrUser && !isa<CallInst>(Curr) && !isa<Constant>(Curr))
         for (auto &Op : CurrUser->operands())
           if (Defs.insert(Op).second)
             WorkList.push_back(Op);
     }
     return Defs;
   }
 
   // Translate a i1 value to an equivalent i32/i64 value:
   Value *translate(Value *V) {
+    assert(V->getType() == Type::getInt1Ty(V->getContext()) &&
+           "Expect an i1 value");
+
     Type *IntTy = ST->isPPC64() ? Type::getInt64Ty(V->getContext())
                                 : Type::getInt32Ty(V->getContext());
 
     if (auto *C = dyn_cast<Constant>(V))
       return ConstantExpr::getZExt(C, IntTy);
     if (auto *P = dyn_cast<PHINode>(V)) {
       // Temporarily set the operands to 0. We'll fix this later in
       // runOnUse.
for (Value *V : Defs)
         BoolToIntMap[V] = translate(V);
 
     // Replace the operands of the translated instructions. They were set to
     // zero in the translate function.
     for (auto &Pair : BoolToIntMap) {
       auto *First = dyn_cast<User>(Pair.first);
       auto *Second = dyn_cast<User>(Pair.second);
       assert((!First || Second) && "translated from user to non-user!?");
-      // Operands of CallInst are skipped because they may not be Bool type,
-      // and their positions are defined by ABI.
-      if (First && !isa<CallInst>(First))
+      // Operands of CallInst/Constant are skipped because they may not be Bool
+      // type. For CallInst, their positions are defined by ABI.
+      if (First && !isa<CallInst>(First) && !isa<Constant>(First))
         for (unsigned i = 0; i < First->getNumOperands(); ++i)
           Second->setOperand(i, BoolToIntMap[First->getOperand(i)]);
     }
 
     Value *IntRetVal = BoolToIntMap[U];
     Type *Int1Ty = Type::getInt1Ty(U->getContext());
     auto *I = cast<Instruction>(U.getUser());
     Value *BackToBool = new TruncInst(IntRetVal, Int1Ty, "backToBool", I);

llvm/test/CodeGen/PowerPC/pr46923.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-unknown \
+; RUN:   -ppc-asm-full-reg-names < %s | FileCheck %s
+
+@bar = external constant i64, align 8
+
+define i1 @foo() {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    li r3, 0
+; CHECK-NEXT:    isel r3, 0, r3, 4*cr5+lt
+; CHECK-NEXT:    blr
+entry:
+  br label %next
+
+next:
+  br i1 undef, label %true, label %false
+
+true:
+  br label %end
+
+false:
+  br label %end
+
+end:
+  %a = phi i1 [ icmp ugt (i64 0, i64 ptrtoint (i64* @bar to i64)), %true ],
+              [ icmp ugt (i64 0, i64 2), %false ]
+  ret i1 %a
+}