[PATCH] D92071: [PowerPC] support register pressure reduction in machine combiner.
Jinsong Ji via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 15 20:03:21 PST 2020
jsji added inline comments.
================
Comment at: llvm/include/llvm/CodeGen/MachineCombinerPattern.h:34
+ // reduce register pressure.
+ REASSOC_FMA_M1C_FSUB,
+ REASSOC_FMA_M2C_FSUB,
----------------
REASSOC_XY_BCA,
REASSOC_XY_BAC,
================
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."));
----------------
"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?
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:83
+
+static cl::opt<bool> EnableRegPressureReduce(
+ "enable-regpressure-reduce", cl::Hidden, cl::init(true),
----------------
EnableFMARegPressureReduction
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:84
+static cl::opt<bool> EnableRegPressureReduce(
+ "enable-regpressure-reduce", cl::Hidden, cl::init(true),
+ cl::desc("enable register pressure reduce in machine combiner pass."));
----------------
"ppc-fma-rp-reduction"
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:350
+// A = FSUB X, Y (Leaf)
+// C = FMA A31, M31, A (Root)
+// M31 is const -->
----------------
// A = FSUB X, Y (Leaf)
// D = FMA B, C, A (Root)
->
// A = FMA B, Y, -C
// C = FMA A, X, C
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:355
+//
+// Pattern 2:
+// A = FSUB X, Y (Leaf)
----------------
This is the same pattern as above, as we can commute the operands of FMA?
// A = FSUB X, Y (Leaf)
// D = FMA B, A, C (Root)
->
// A = FMA B, Y, -C
// C = FMA A, X, C
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:442
+ // Register pressure fma reassociation patterns.
+ if (DoRegPressureReduce && isRegisterPressureReduceCandidate(Root)) {
+ // Root must be a valid FMA like instruction.
----------------
Do all candidate checks in isRegisterPressureReduceCandidate, so that we have simple logic like:
```
if (DoRegPressureReduce && isRegisterPressureReduceCandidate(Root, MULInstrL,MULInstrR )) {
if (isLoadFromConstantPool(MULInstrL) &&
IsReassociableAddOrSub(*MULInstrR, InfoArrayIdxFSubInst)) {
LLVM_DEBUG(dbgs() << "add pattern REASSOC_XY_BCA\n");
Patterns.push_back(MachineCombinerPattern::REASSOC_XY_BCA);
return true;
}
if (isLoadFromConstantPool(MULInstrR) &&
IsReassociableAddOrSub(*MULInstrL, InfoArrayIdxFSubInst)) {
LLVM_DEBUG(dbgs() << "add pattern REASSOC_XY_BCA\n");
Patterns.push_back(MachineCombinerPattern::REASSOC_XY_BCA);
return true;
}
}
```
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:445
+ // Treat it as leaf as we don't care its add operand.
+ if (IsReassociableFMA(Root, AddOpIdx, MulOpIdx, true)) {
+ assert((MulOpIdx >= 0) && "mul operand index not right!");
----------------
This check should be in `isRegisterPressureReduceCandidate`
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:447
+ assert((MulOpIdx >= 0) && "mul operand index not right!");
+ bool IsUsedOnceL;
+ bool IsUsedOnceR;
----------------
Uninitialized variable.
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:449
+ bool IsUsedOnceR;
+ Register MULRegL = TRI->lookThruCopyLike(
+ Root.getOperand(MulOpIdx).getReg(), MRI, &IsUsedOnceL);
----------------
This should be in `isRegisterPressureReduceCandidate` as well.
================
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);
----------------
Why this can not be inlined into next line of ConstantFP::get?
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:569
+
+ assert(Placeholder && "Placeholder does not exist!");
+
----------------
What if we are in non-assert version and we can't find `Placeholder` ?
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:602
+
+ RegionPressure Pressure;
+ RegPressureTracker RPTracker(Pressure);
----------------
Split this out into a lamda or function , just call something like
```
currMBBPresure = getMBBPressure (MBB...);
VSSRCRPSetLimit= ...
return currMBBPresure >= VSSRCRPSetLimit * RegPressureFactor
```
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:637
+
+bool PPCInstrInfo::isRegisterPressureReduceCandidate(MachineInstr &Root) const {
+ // Currently, we only support float and double.
----------------
This should check the Root, and look through copies, isVirtualRegister.
Only return true when all the requirements are met.
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:705
+
+const Constant *
+PPCInstrInfo::getConstantFromConstantPool(MachineInstr *I) const {
----------------
Add document about function and argument assumptions.
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:787
+ MachineInstr *Leaf = nullptr;
+ if (IsILPReassociate) {
+ Prev = MRI.getUniqueVRegDef(Root.getOperand(AddOpIdx).getReg());
----------------
I think a switch table here would be clearer.
```
switch (Pattern){
case: MachineCombinerPattern::REASSOC_XY_AMM_BMM:
case :MachineCombinerPattern::REASSOC_XY_AMM_BMM:
IsILPReassociate = true;
Prev = MRI.getUniqueVRegDef(Root.getOperand(AddOpIdx).getReg());
Leaf = MRI.getUniqueVRegDef(Prev->getOperand(AddOpIdx).getReg());
IntersectedFlags = Root.getFlags() & Prev->getFlags() & Leaf->getFlags();
break;
case: MachineCombinerPattern::REASSOC_XY_BAC:
...
break;
}
```
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:803
+
+ uint16_t IntersectedFlags;
+ if (IsILPReassociate)
----------------
Uninitialized variable.
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:882
+
if (Pattern == MachineCombinerPattern::REASSOC_XY_AMM_BMM) {
// Create new instructions for insertion.
----------------
Switch table please as well.
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:255
DenseMap<unsigned, unsigned> &InstrIdxForVirtReg) const;
+ bool isRegisterPressureReduceCandidate(MachineInstr &Root) const;
+ bool isLoadFromConstantPool(MachineInstr *I) const;
----------------
isRPReductionCandidate
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:353
+ SmallVectorImpl<MachineCombinerPattern> &P,
+ bool DoRegPressureReduce) const;
----------------
DoRPReduction
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:362
+ /// On PowerPC, we leverage machine combiner pass to reduce register pressure
+ /// when the register pressure is high for one BB.
----------------
/// Return true when ...
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:368
+
+ /// Fixup the placeholders we put in genAlternativeCodeSequence().
+ void
----------------
Fixup the placeholders we put in genAlternativeCodeSequence() for MachineCombiner.
================
Comment at: llvm/test/CodeGen/PowerPC/register-pressure-reduction.ll:9
+
+define float @foo_float(float %0, float %1, float %2, float %3) {
+; CHECK-LABEL: foo_float:
----------------
Can we add test to see whether we will be able to reuse const?
eg: If there is already negative const in the const pool.
================
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:
----------------
If we have 2 patterns, we should test both patterns..
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92071/new/
https://reviews.llvm.org/D92071
More information about the llvm-commits
mailing list