[PowerPC] add store (load float*) pattern to isProfitableToHoist

[shchenz]

Summary

store (load float*) can be optimized to store(load i32*) in InstCombine pass.

Add store (load float*) to isProfitableToHoist to make sure we don't break the opt in InstCombine pass.

[shchenz]

gentle ping

[shchenz]

gentle ping...

[jsji]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

16384

Looks like the FIXME is wrong? The default is true, so we should be adding more patterns which are NOT profitable to hoist instead?

16399–16433

This is also common, can be moved before switch.

16407

!I->hasOneUse() is common for all, can we move it up before switch?
Also the Type check can be done after Store opcode check?

16410

If these conditions are *copied* from combineLoadToOperationType, please also copy comments.
Otherwise please add comments about why we need these condition, especially isSwiftError?

Also do we have any IR test with SwiftError?

llvm/test/Transforms/SimplifyCFG/PowerPC/prefer-load-i32.ll

1

Precommit the new testcase first so that we can see the diff here.

[shchenz]

address @jsji comments:
1: code refactor
2: split out NFC patch for new added testcase

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

16410

SwiftError attribute can only be applied to pointer to pointer type, so I->getType()->getTypeID() != Type::FloatTyID should get rid of this case.

[jsji]

LGTM.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

case Type::DoubleTyID:
     return true;
   case Type::FP128TyID:
     return Subtarget.hasP9Vector();
   default:
     return false;
   }
 }
 
-// Currently this is a copy from AArch64TargetLowering::isProfitableToHoist.
-// FIXME: add more patterns which are profitable to hoist.
+// FIXME: add more patterns which are not profitable to hoist.
 bool PPCTargetLowering::isProfitableToHoist(Instruction *I) const {
-  if (I->getOpcode() != Instruction::FMul)
-    return true;
-
   if (!I->hasOneUse())
     return true;
 
   Instruction *User = I->user_back();
   assert(User && "A single use instruction with no uses.");
 
+  switch (I->getOpcode()) {
+  case Instruction::FMul: {
+    // Don't break FMA, PowerPC prefers FMA.
     if (User->getOpcode() != Instruction::FSub &&
         User->getOpcode() != Instruction::FAdd)
       return true;
 
     const TargetOptions &Options = getTargetMachine().Options;
     const Function *F = I->getFunction();
     const DataLayout &DL = F->getParent()->getDataLayout();
     Type *Ty = User->getOperand(0)->getType();
 
     return !(
         isFMAFasterThanFMulAndFAdd(*F, Ty) &&
         isOperationLegalOrCustom(ISD::FMA, getValueType(DL, Ty)) &&
         (Options.AllowFPOpFusion == FPOpFusion::Fast || Options.UnsafeFPMath));
   }
+  case Instruction::Load: {
+    // Don't break "store (load float*)" pattern, this pattern will be combined
+    // to "store (load int32)" in later InstCombine pass. See function
+    // combineLoadToOperationType. On PowerPC, loading a float point takes more
+    // cycles than loading a 32 bit integer.
+    LoadInst *LI = cast<LoadInst>(I);
+    // For the loads that combineLoadToOperationType does nothing, like
+    // ordered load, it should be profitable to hoist them.
+    // For swifterror load, it can only be used for pointer to pointer type, so
+    // later type check should get rid of this case.
+    if (!LI->isUnordered())
+      return true;
+
+    if (User->getOpcode() != Instruction::Store)
+      return true;
+
+    if (I->getType()->getTypeID() != Type::FloatTyID)
+      return true;
+
+    return false;
+  }
+  default:
+    return true;
+  }
+  return true;
+}
 
 const MCPhysReg *
 PPCTargetLowering::getScratchRegisters(CallingConv::ID) const {
   // LR is a callee-save register, but we must treat it as clobbered by any call
   // site. Hence we include LR in the scratch registers, which are in turn added
   // as implicit-defs for stackmaps and patchpoints. The same reasoning applies
   // to CTR, which is used by any indirect call.
   static const MCPhysReg ScratchRegs[] = {

llvm/test/Transforms/SimplifyCFG/PowerPC/prefer-load-i32.ll

 ; RUN: opt < %s -mtriple=powerpc64le-unknown-linux-gnu -simplifycfg -hoist-common-insts=true -S | FileCheck %s
 
 define float @foo(float* %src, float* %dest, i32 signext %count, i32 signext %cond) {
 ; CHECK-LABEL: @foo(
 ; CHECK-LABEL: entry:
-; CHECK:  %0 = load float, float* %arrayidx, align 4
+; CHECK-NOT:  load float
 entry:
   %cmp = icmp sgt i32 %cond, 10
   %idxprom = sext i32 %count to i64
   %arrayidx = getelementptr inbounds float, float* %src, i64 %idxprom
   br i1 %cmp, label %if.then, label %if.else
 
 ; CHECK-LABEL: if.then:
-; CHECK-NOT:   load float
+; CHECK:  %0 = load float, float* %arrayidx, align 4
 if.then:                                          ; preds = %entry
   %0 = load float, float* %arrayidx, align 4
   %res = fmul float %0, 3.000000e+00
   br label %if.end
 
 ; CHECK-LABEL: if.else:
-; CHECK-NOT:   load float
+; CHECK:   %1 = load float, float* %arrayidx, align 4
+; CHECK:   store float %1, float* %arrayidx4, align 4
 if.else:                                          ; preds = %entry
   %1 = load float, float* %arrayidx, align 4
   %idxprom3 = sext i32 %count to i64
   %arrayidx4 = getelementptr inbounds float, float* %dest, i64 %idxprom3
   store float %1, float* %arrayidx4, align 4
   br label %if.end
 
 if.end:                                           ; preds = %if.else, %if.then
   %res2.0 = phi float [ %res, %if.then ], [ 0.000000e+00, %if.else ]
   ret float %res2.0
 }