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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15997
 
-// Currently this is a copy from AArch64TargetLowering::isProfitableToHoist.
 // FIXME: add more patterns which are profitable to hoist.
 bool PPCTargetLowering::isProfitableToHoist(Instruction *I) const {
----------------
Looks like the FIXME is wrong? The default is true, so we should be adding more patterns which are NOT profitable to hoist instead?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16027
+    // cycles than loading a 32 bit integer.
+    if (!I->hasOneUse() || I->getType()->getTypeID() != Type::FloatTyID)
+      return true;
----------------
`!I->hasOneUse()` is common for all, can we move it up before `switch`?
Also the Type check can be done after Store opcode check? 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16030
+    LoadInst *LI = cast<LoadInst>(I);
+    if (!LI->isUnordered() || LI->getPointerOperand()->isSwiftError())
+      return true;
----------------
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?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16033
 
-  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));
+    Instruction *User = I->user_back();
+    assert(User && "A single use instruction with no uses.");
----------------
This is also common, can be moved before switch.


================
Comment at: llvm/test/Transforms/SimplifyCFG/PowerPC/prefer-load-i32.ll:1
+; RUN: opt < %s -mtriple=powerpc64le-unknown-linux-gnu -simplifycfg -S | \
+; RUN: FileCheck %s
----------------
Precommit the new testcase first so that we can see the diff here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82341/new/

https://reviews.llvm.org/D82341