[PPC] In PPCBoolRetToInt change the bool value to i64 if the target is ppc64

[Carrot]

Summary

In PPCBoolRetToInt bool value is changed to i32 type. On ppc64 it may introduce an extra zero extension for the return value. This patch changes the integer type to i64 to avoid the zero extension on ppc64.

This patch fixed PR32442.

[echristo]

Couple of small nits, otherwise LGTM.

-eric

lib/Target/PowerPC/PPCBoolRetToInt.cpp

91 ↗

(On Diff #93189)

Update?

197 ↗

(On Diff #93189)

Let's just always assume we have a TargetMachine and return false if not?

test/CodeGen/PowerPC/pr32442.ll

2 ↗

(On Diff #93189)

Throw the PR number in here as a comment?

[nemanjai]

lib/Target/PowerPC/PPCBoolRetToInt.cpp

93 ↗

(On Diff #93189)

Just a personal preference nit...
Maybe turn this into a conditional operator for the initialization (rather than the assignment in the if).

[Carrot]

test/CodeGen/PowerPC/pr32442.ll

2 ↗

(On Diff #93189)

It is contained in the filename.

[echristo]

test/CodeGen/PowerPC/pr32442.ll

2 ↗

(On Diff #93189)

So it is. Perhaps a more descriptive filename and a link to the PR :)

[echristo]

test/CodeGen/PowerPC/pr32442.ll

2 ↗

(On Diff #93189)

Any reason this can't go into the other file?

[Carrot]

test/CodeGen/PowerPC/pr32442.ll

2 ↗

(On Diff #93189)

Test case BoolRetToIntTest.ll runs BoolRetToInt pass only, and check the LLVM IR. The extra zero extension is not very obvious at this time, it is explicitly generated at the machine instruction phase.

[echristo]

test/CodeGen/PowerPC/pr32442.ll

2 ↗

(On Diff #93189)

Aha. That makes sense.

How about BoolRetToIntTest-2.ll then and leave the PR in the comment section?

I realize this seems like a crazy amount of care over file naming, but historically I've found that pr named tests aren't very helpful.

[echristo]

LGTM.

Thanks!

[Carrot]

The first commit triggered bug pr32608, and was reverted by r299153. Now the bug has been fixed by r304001, so I will try to commit it again. Unfortunately INITIALIZE_TM_PASS has been removed and TargetMachine access mechanism is changed, so I modified the patch to use new mechanism.

Please take another look.

[echristo]

One inline nit then LGTM.

-eric

lib/Target/PowerPC/PPCBoolRetToInt.cpp

197 ↗

(On Diff #93189)

If you get a PPCTargetMachine you shouldn't have to cast the subtarget.

llvm/trunk/lib/Target/PowerPC/PPC.h

#endif
   FunctionPass *createPPCVSXCopyPass();
   FunctionPass *createPPCVSXFMAMutatePass();
   FunctionPass *createPPCVSXSwapRemovalPass();
   FunctionPass *createPPCMIPeepholePass();
   FunctionPass *createPPCBranchSelectionPass();
   FunctionPass *createPPCQPXLoadSplatPass();
   FunctionPass *createPPCISelDag(PPCTargetMachine &TM);
   FunctionPass *createPPCTLSDynamicCallPass();
-  FunctionPass *createPPCBoolRetToIntPass();
+  FunctionPass *createPPCBoolRetToIntPass(PPCTargetMachine *TM);
   FunctionPass *createPPCExpandISELPass();
   void LowerPPCMachineInstrToMCInst(const MachineInstr *MI, MCInst &OutMI,
                                     AsmPrinter &AP, bool isDarwin);
 
   void initializePPCVSXFMAMutatePass(PassRegistry&);
   void initializePPCBoolRetToIntPass(PassRegistry&);
   void initializePPCExpandISELPass(PassRegistry &);
   extern char &PPCVSXFMAMutateID;

llvm/trunk/lib/Target/PowerPC/PPCBoolRetToInt.cpp

 //===- PPCBoolRetToInt.cpp ------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
-// This file implements converting i1 values to i32 if they could be more
+// This file implements converting i1 values to i32/i64 if they could be more
 // profitably allocated as GPRs rather than CRs. This pass will become totally
 // unnecessary if Register Bank Allocation and Global Instruction Selection ever
 // go upstream.
 //
-// Presently, the pass converts i1 Constants, and Arguments to i32 if the
+// Presently, the pass converts i1 Constants, and Arguments to i32/i64 if the
 // transitive closure of their uses includes only PHINodes, CallInsts, and
 // ReturnInsts. The rational is that arguments are generally passed and returned
-// in GPRs rather than CRs, so casting them to i32 at the LLVM IR level will
+// in GPRs rather than CRs, so casting them to i32/i64 at the LLVM IR level will
 // actually save casts at the Machine Instruction level.
 //
 // It might be useful to expand this pass to add bit-wise operations to the list
 // of safe transitive closure types. Also, we miss some opportunities when LLVM
 // represents logical AND and OR operations with control flow rather than data
 // flow. For example by lowering the expression: return (A && B && C)
 //
 // as: return A ? true : B && C.
 //
 // There's code in SimplifyCFG that code be used to turn control flow in data
 // flow using SelectInsts. Selects are slow on some architectures (P7/P8), so
 // this probably isn't good in general, but for the special case of i1, the
 // Selects could be further lowered to bit operations that are fast everywhere.
 //
 //===----------------------------------------------------------------------===//
 
 #include "PPC.h"
+#include "PPCTargetMachine.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/Argument.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Dominators.h"
while (!WorkList.empty()) {
       if (CurrUser && !isa<CallInst>(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 value:
-  static Value *translate(Value *V) {
-    Type *Int32Ty = Type::getInt32Ty(V->getContext());
+  // Translate a i1 value to an equivalent i32/i64 value:
+  Value *translate(Value *V) {
+    Type *IntTy = ST->isPPC64() ? Type::getInt64Ty(V->getContext())
+                                : Type::getInt32Ty(V->getContext());
+
     if (auto *C = dyn_cast<Constant>(V))
-      return ConstantExpr::getZExt(C, Int32Ty);
+      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.
-      Value *Zero = Constant::getNullValue(Int32Ty);
+      Value *Zero = Constant::getNullValue(IntTy);
       PHINode *Q =
-        PHINode::Create(Int32Ty, P->getNumIncomingValues(), P->getName(), P);
+        PHINode::Create(IntTy, P->getNumIncomingValues(), P->getName(), P);
       for (unsigned i = 0; i < P->getNumOperands(); ++i)
         Q->addIncoming(Zero, P->getIncomingBlock(i));
       return Q;
     }
 
     auto *A = dyn_cast<Argument>(V);
     auto *I = dyn_cast<Instruction>(V);
     assert((A || I) && "Unknown value type");
 
     auto InstPt =
       A ? &*A->getParent()->getEntryBlock().begin() : I->getNextNode();
-    return new ZExtInst(V, Int32Ty, "", InstPt);
+    return new ZExtInst(V, IntTy, "", InstPt);
   }
 
   typedef SmallPtrSet<const PHINode *, 8> PHINodeSet;
 
   // A PHINode is Promotable if:
   // 1. Its type is i1 AND
   // 2. All of its uses are ReturnInt, CallInst, PHINode, or DbgInfoIntrinsic
   // AND
static PHINodeSet getPromotablePHINodes(const Function &F) {
     return Promotable;
   }
 
   typedef DenseMap<Value *, Value *> B2IMap;
 
  public:
   static char ID;
 
-  PPCBoolRetToInt() : FunctionPass(ID) {
+  PPCBoolRetToInt() : FunctionPass(ID), TM(nullptr) {
+    initializePPCBoolRetToIntPass(*PassRegistry::getPassRegistry());
+  }
+
+  PPCBoolRetToInt(TargetMachine *&TM) : FunctionPass(ID), TM(TM) {
     initializePPCBoolRetToIntPass(*PassRegistry::getPassRegistry());
   }
 
   bool runOnFunction(Function &F) override {
     if (skipFunction(F))
       return false;
 
+    if (!TM)
+      return false;
+    ST = ((PPCTargetMachine*)TM)->getSubtargetImpl(F);
+
     PHINodeSet PromotablePHINodes = getPromotablePHINodes(F);
     B2IMap Bool2IntMap;
     bool Changed = false;
     for (auto &BB : F) {
       for (auto &I : BB) {
         if (auto *R = dyn_cast<ReturnInst>(&I))
           if (F.getReturnType()->isIntegerTy(1))
             Changed |=
               runOnUse(R->getOperandUse(0), PromotablePHINodes, Bool2IntMap);
 
         if (auto *CI = dyn_cast<CallInst>(&I))
           for (auto &U : CI->operands())
             if (U->getType()->isIntegerTy(1))
               Changed |= runOnUse(U, PromotablePHINodes, Bool2IntMap);
       }
     }
 
     return Changed;
   }
 
-  static bool runOnUse(Use &U, const PHINodeSet &PromotablePHINodes,
+  bool runOnUse(Use &U, const PHINodeSet &PromotablePHINodes,
                        B2IMap &BoolToIntMap) {
     auto Defs = findAllDefs(U);
 
     // If the values are all Constants or Arguments, don't bother
     if (llvm::none_of(Defs, isa<Instruction, Value *>))
       return false;
 
     // Presently, we only know how to handle PHINode, Constant, Arguments and
bool runOnUse(Use &U, const PHINodeSet &PromotablePHINodes,
 
     return true;
   }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addPreserved<DominatorTreeWrapperPass>();
     FunctionPass::getAnalysisUsage(AU);
   }
+
+private:
+  const PPCSubtarget *ST;
+  TargetMachine *TM;
 };
 
 } // end anonymous namespace
 
 char PPCBoolRetToInt::ID = 0;
-INITIALIZE_PASS(PPCBoolRetToInt, "bool-ret-to-int",
-                "Convert i1 constants to i32 if they are returned",
+INITIALIZE_TM_PASS(PPCBoolRetToInt, "bool-ret-to-int",
+                   "Convert i1 constants to i32/i64 if they are returned",
                    false, false)
 
-FunctionPass *llvm::createPPCBoolRetToIntPass() { return new PPCBoolRetToInt(); }
+FunctionPass *llvm::createPPCBoolRetToIntPass(PPCTargetMachine *TM) {
+  TargetMachine *pTM = TM;
+  return new PPCBoolRetToInt(pTM);
+}

llvm/trunk/lib/Target/PowerPC/PPCTargetMachine.cpp

 } // end anonymous namespace
 
 TargetPassConfig *PPCTargetMachine::createPassConfig(PassManagerBase &PM) {
   return new PPCPassConfig(this, PM);
 }
 
 void PPCPassConfig::addIRPasses() {
   if (TM->getOptLevel() != CodeGenOpt::None)
-    addPass(createPPCBoolRetToIntPass());
+    addPass(createPPCBoolRetToIntPass(&getPPCTargetMachine()));
   addPass(createAtomicExpandPass(&getPPCTargetMachine()));
 
   // For the BG/Q (or if explicitly requested), add explicit data prefetch
   // intrinsics.
   bool UsePrefetching = TM->getTargetTriple().getVendor() == Triple::BGQ &&
                         getOptLevel() != CodeGenOpt::None;
   if (EnablePrefetch.getNumOccurrences() > 0)
     UsePrefetching = EnablePrefetch;

llvm/trunk/test/CodeGen/PowerPC/BoolRetToIntTest-2.ll

+; RUN: llc -mtriple=powerpc64le-linux-gnu -mcpu=pwr8 < %s | FileCheck %s
+
+; https://bugs.llvm.org/show_bug.cgi?id=32442
+; Don't generate zero extension for the return value.
+; CHECK-NOT: clrldi
+
+define zeroext i1 @foo(i32 signext %i, i32* %p) {
+entry:
+  %cmp = icmp eq i32 %i, 0
+  br i1 %cmp, label %return, label %if.end
+
+if.end:
+  store i32 %i, i32* %p, align 4
+  br label %return
+
+return:
+  %retval = phi i1 [ true, %if.end ], [ false, %entry ]
+  ret i1 %retval
+}
+

llvm/trunk/test/CodeGen/PowerPC/BoolRetToIntTest.ll

 for.body:                                         ; preds = %for.body.preheader, %for.cond
   %curr.05 = phi i8** [ %incdec.ptr, %for.cond ], [ %begin, %for.body.preheader ]
   %0 = load i8*, i8** %curr.05, align 8
   %call = tail call zeroext i1 %hasProp(i8* %0)
   %incdec.ptr = getelementptr inbounds i8*, i8** %curr.05, i64 1
   br i1 %call, label %cleanup.loopexit, label %for.cond
 
 cleanup.loopexit:                                 ; preds = %for.body, %for.cond
-; CHECK: [[PHI:%.+]] = phi i32 [ 1, %for.body ], [ 0, %for.cond ]
+; CHECK: [[PHI:%.+]] = phi i64 [ 1, %for.body ], [ 0, %for.cond ]
   %cleanup.dest.slot.0.ph = phi i1 [ true, %for.body ], [ false, %for.cond ]
   br label %cleanup
 
 cleanup:                                          ; preds = %cleanup.loopexit, %entry
-; CHECK: = phi i32 [ 0, %entry ], [ [[PHI]], %cleanup.loopexit ]
+; CHECK: = phi i64 [ 0, %entry ], [ [[PHI]], %cleanup.loopexit ]
   %cleanup.dest.slot.0 = phi i1 [ false, %entry ], [ %cleanup.dest.slot.0.ph, %cleanup.loopexit ]
-; CHECK: [[REG:%.+]] = trunc i32 {{%.+}} to i1
+; CHECK: [[REG:%.+]] = trunc i64 {{%.+}} to i1
 ; CHECK: ret i1 [[REG]]
   ret i1 %cleanup.dest.slot.0
 }
 
 ; CHECK-LABEL: retFalse
 define zeroext i1 @retFalse() {
 entry:
 ; CHECK: ret i1 false

 for.body:                                         ; preds = %for.body.preheader, %for.cond
   %curr.05 = phi i8** [ %incdec.ptr, %for.cond ], [ %begin, %for.body.preheader ]
   %0 = load i8*, i8** %curr.05, align 8
   %call = tail call zeroext i1 %hasProp(i8* %0)
   %incdec.ptr = getelementptr inbounds i8*, i8** %curr.05, i64 1
   br i1 %call, label %cleanup.loopexit, label %for.cond
 
 cleanup.loopexit:                                 ; preds = %for.body, %for.cond
-; CHECK: [[PHI:%.+]] = phi i32 [ 1, %for.body ], [ 0, %for.cond ]
+; CHECK: [[PHI:%.+]] = phi i64 [ 1, %for.body ], [ 0, %for.cond ]
   %cleanup.dest.slot.0.ph = phi i1 [ true, %for.body ], [ false, %for.cond ]
   br label %cleanup
 
 cleanup:                                          ; preds = %cleanup.loopexit, %entry
-; CHECK: = phi i32 [ 0, %entry ], [ [[PHI]], %cleanup.loopexit ]
+; CHECK: = phi i64 [ 0, %entry ], [ [[PHI]], %cleanup.loopexit ]
   %cleanup.dest.slot.0 = phi i1 [ false, %entry ], [ %cleanup.dest.slot.0.ph, %cleanup.loopexit ]
-; CHECK: [[REG:%.+]] = trunc i32 {{%.+}} to i1
+; CHECK: [[REG:%.+]] = trunc i64 {{%.+}} to i1
 ; CHECK: call void %cont(i1 [[REG]]
   tail call void %cont(i1 %cleanup.dest.slot.0)
   ret void
 }
 
 ; CHECK-LABEL: find_cont_ret
 define zeroext i1 @find_cont_ret(i8** readonly %begin, i8** readnone %end, i1 (i8*)* nocapture %hasProp, void (i1)* nocapture %cont) {
 entry:

 for.body:                                         ; preds = %for.body.preheader, %for.cond
   %curr.05 = phi i8** [ %incdec.ptr, %for.cond ], [ %begin, %for.body.preheader ]
   %0 = load i8*, i8** %curr.05, align 8
   %call = tail call zeroext i1 %hasProp(i8* %0)
   %incdec.ptr = getelementptr inbounds i8*, i8** %curr.05, i64 1
   br i1 %call, label %cleanup.loopexit, label %for.cond
 
 cleanup.loopexit:                                 ; preds = %for.body, %for.cond
-; CHECK: [[PHI:%.+]] = phi i32 [ 1, %for.body ], [ 0, %for.cond ]
+; CHECK: [[PHI:%.+]] = phi i64 [ 1, %for.body ], [ 0, %for.cond ]
   %cleanup.dest.slot.0.ph = phi i1 [ true, %for.body ], [ false, %for.cond ]
   br label %cleanup
 
 cleanup:                                          ; preds = %cleanup.loopexit, %entry
-; CHECK: = phi i32 [ 0, %entry ], [ [[PHI]], %cleanup.loopexit ]
+; CHECK: = phi i64 [ 0, %entry ], [ [[PHI]], %cleanup.loopexit ]
   %cleanup.dest.slot.0 = phi i1 [ false, %entry ], [ %cleanup.dest.slot.0.ph, %cleanup.loopexit ]
-; CHECK: [[REG:%.+]] = trunc i32 {{%.+}} to i1
+; CHECK: [[REG:%.+]] = trunc i64 {{%.+}} to i1
 ; CHECK: call void %cont(i1 [[REG]]
   tail call void %cont(i1 %cleanup.dest.slot.0)
-; CHECK: [[REG:%.+]] = trunc i32 {{%.+}} to i1
+; CHECK: [[REG:%.+]] = trunc i64 {{%.+}} to i1
 ; CHECK: ret i1 [[REG]]
   ret i1 %cleanup.dest.slot.0
 }
 
 ; CHECK-LABEL: arg_operand
 define zeroext i1 @arg_operand(i1 %operand) {
 entry:
   br i1 %operand, label %foo, label %cleanup
 
 foo:
   br label %cleanup
 
 cleanup:
-; CHECK: [[REG:%.+]] = trunc i32 {{%.+}} to i1
+; CHECK: [[REG:%.+]] = trunc i64 {{%.+}} to i1
 ; CHECK: ret i1 [[REG]]
   %result = phi i1 [ false, %foo ], [ %operand, %entry ]
   ret i1 %result
 }
 
 ; CHECK-LABEL: bad_use
 define zeroext i1 @bad_use(i1 %operand) {
 entry:
entry:
   br i1 %operand, label %foo, label %cleanup
 
 foo:
   %bar = phi i1 [ false, %entry ]
   br label %cleanup
 
 ; CHECK-LABEL: cleanup
 cleanup:
-; CHECK: [[REG:%.+]] = trunc i32 {{%.+}} to i1
+; CHECK: [[REG:%.+]] = trunc i64 {{%.+}} to i1
 ; CHECK: ret i1 [[REG]]
   %result = phi i1 [ %bar, %foo], [ %operand, %entry ]
   ret i1 %result
 }
 
 declare zeroext i1 @return_i1()
 
 ; CHECK-LABEL: call_test
 define zeroext i1 @call_test() {
 ; CHECK: [[REG:%.+]] = call i1
   %result = call i1 @return_i1()
-; CHECK: [[REG:%.+]] = zext i1 {{%.+}} to i32
-; CHECK: [[REG:%.+]] = trunc i32 {{%.+}} to i1
+; CHECK: [[REG:%.+]] = zext i1 {{%.+}} to i64
+; CHECK: [[REG:%.+]] = trunc i64 {{%.+}} to i1
 ; CHECK: ret i1 [[REG]]
   ret i1 %result
 }