[PATCH] D43086: [PowerPC] Infrastructure work. Implement getting the opcode for a spill in one place.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

This is starting to take shape quite nicely. I suspect you're planning a similar refactoring for the loads?



================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:248
+def LDARX : XForm_1_memOp<31,  84, (outs g8rc:$rD), (ins memrr:$ptr),
                     "ldarx $rD, $ptr", IIC_LdStLDARX, []>;
 
----------------
For all of these, you need to move the line below so that the `"` lines up under the `3` (i.e. parameters on multiple lines start at the same column).


================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:1160
 
-def STBUX8: XForm_8<31, 247, (outs ptr_rc_nor0:$ea_res), (ins g8rc:$rS, memrr:$dst),
+def STBUX8: XForm_8_memOp<31, 247, (outs ptr_rc_nor0:$ea_res), (ins g8rc:$rS, memrr:$dst),
                     "stbux $rS, $dst", IIC_LdStStoreUpd, []>,
----------------
Line too long.


================
Comment at: lib/Target/PowerPC/PPCInstrFormats.td:117
 
+// Base class for all X-Form instructions
+class IXFormMemOp<bits<6> opcode, dag OOL, dag IOL, string asmstr, InstrItinClass itin>
----------------
`// Base class for all X-Form memory instructions`


================
Comment at: lib/Target/PowerPC/PPCInstrFormats.td:449
 
+class XForm_base_r3xo_memOp<bits<6> opcode, bits<10> xo, dag OOL, dag IOL, string asmstr, 
+                      InstrItinClass itin, list<dag> pattern>
----------------
This line is too long. In any case, do we have to redefine this? Seems like just copy-paste. Can we not just inherit from both `XForm_base_r3xo` and `XFormMemOp` without a re-definition?


================
Comment at: lib/Target/PowerPC/PPCInstrFormats.td:1066
 
+class XX1Form_memOp<bits<6> opcode, bits<10> xo, dag OOL, dag IOL, string asmstr, 
+              InstrItinClass itin, list<dag> pattern>
----------------
Similarly here. Do we have to provide a definition or can we just inherit from `XX1Form` and `XFormMemOp`?

But perhaps I've missed something.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1093
+    SmallVectorImpl<MachineInstr *> &NewMIs) const {
+  unsigned Opcode = getOpcodeForSpill(0, RC);
+  DebugLoc DL;
----------------
I prefer that we make arguments self-documenting when possible. Please instead of `0`, pass `PPC::NoRegister`.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1112
+
+  if (MCII->get(Opcode).TSFlags & PPCII::XFormMemOp)
+    FuncInfo->setHasNonRISpills();
----------------
This seems like a strange way to get to the flags. Can you not just `get(Opcode).TSFlags` since you're in `PPCInstrInfo` already?


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:2318
+
+  return OpcodesForSpill[Subtarget.hasP9Vector()];
+}
----------------
Please use a ternary operator here with 0/1. They should be equivalent, but I've come across UBSan failures where we load other values - so until we figure out whether that's a PPC problem or not, I'd prefer being explicit about this.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.td:2025
 let PPC970_Unit = 2, mayStore = 1, mayLoad = 0 in {
-def STBUX : XForm_8<31, 247, (outs ptr_rc_nor0:$ea_res), (ins gprc:$rS, memrr:$dst),
+def STBUX : XForm_8_memOp<31, 247, (outs ptr_rc_nor0:$ea_res), (ins gprc:$rS, memrr:$dst),
                     "stbux $rS, $dst", IIC_LdStStoreUpd, []>,
----------------
Line too long. I probably won't notice all of them, but please be careful about this.


https://reviews.llvm.org/D43086