[PATCH] D92071: [PowerPC] support register pressure reduction in machine combiner.

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:80
+static cl::opt<float> RegPressureThreshold(
+    "reg-pressure-threshold", cl::Hidden, cl::init(1.0),
+    cl::desc("register pressure threshold for the transformations."));
----------------
jsji wrote:
> "ppc-fma-rp-factor"
> 
> Since we multiple this value, this is more a factor than a threshold, then the default value should be slightly more than 1.0?
> 
> 
The logic here is if the calculated register pressure(`RPTracker.getPressure()`) exceeds the limits(get`RegPressureSetLimit`), we should do the reassociation. So I used 1.0. Does changing the compare from `>=` to `>` make sense to you?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:550
+  APFloat F1((dyn_cast<ConstantFP>(C))->getValueAPF());
+  F1.changeSign();
+  Constant *NegC = ConstantFP::get(dyn_cast<ConstantFP>(C)->getContext(), F1);
----------------
jsji wrote:
> Why this can not be inlined into next line of ConstantFP::get?
return type of `F1.changeSign();` is `void`


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:569
+
+  assert(Placeholder && "Placeholder does not exist!");
+
----------------
jsji wrote:
> What if we are in non-assert version and we can't find `Placeholder` ?
if we are in non-assert version, we should get SEGV, we use `Placeholder` like `Placeholder->setReg(LoadNewConst);`. If we come here, `Placeholder` must be not null, we only handle register reduction patterns here.

Do we need to add `if(!Placeholder) return;`? Seems we do not do this kind of protection in current llvm code base, for example:
```
ICmpInst::Predicate Loop::LoopBounds::getCanonicalPredicate() const {
  BasicBlock *Latch = L.getLoopLatch();
  assert(Latch && "Expecting valid latch");

  BranchInst *BI = dyn_cast_or_null<BranchInst>(Latch->getTerminator());
``` 


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:637
+
+bool PPCInstrInfo::isRegisterPressureReduceCandidate(MachineInstr &Root) const {
+  // Currently, we only support float and double.
----------------
jsji wrote:
> This should check the Root, and look through copies, isVirtualRegister.
> Only return true when all the requirements are met.
new `IsRPReductionCandidate` already does more than this.


================
Comment at: llvm/test/CodeGen/PowerPC/register-pressure-reduction.ll:45
+
+define double @foo_double(double %0, double %1, double %2, double %3) {
+; CHECK-LABEL: foo_double:
----------------
jsji wrote:
> If we have 2 patterns, we should test both patterns..
I tried this before, seems in ISel, constant operand is put as the second mul operand in FMA instruction. I also can not create a MIR case, as there are const pool inputs, met some syntax errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92071