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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

Thank you for your patience with this review cycle and for cleaning this up. It looks much better now and it's really nice not to have to maintain lists of opcodes in multiple places that have to be kept in sync.

Other than the few minor nits which you should feel free to fix on the commit, LGTM.



================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:841
+def LHAX8: XForm_1_memOp<31, 343, (outs g8rc:$rD), (ins memrr:$src),
                    "lhax $rD, $src", IIC_LdStLHA,
                    [(set i64:$rD, (sextloadi16 xaddr:$src))]>,
----------------
It seems that you've fixed the indentation on the above, but not on any of these. Please indent the continuation line so that the first character (`"`) is under the first character of the parameter in the line above (`3`).


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:91
+  SOK_SpillToVSR,
+  SOK_LastOpcodeSpill  // This must be last on the enum
+};
----------------
Don't forget periods on comments - complete sentences.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1077
 
-  return false;
+unsigned PPCInstrInfo::getLoadOpcodeForSpill(unsigned Reg,
+                                            const TargetRegisterClass *RC)
----------------
Something is off in the formatting of the signature. Please run clang-format on the new code you've added. I think you can run `clang-format-diff` on the actual diff, but I haven't tried that.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1180
+
+  if (get(Opcode).TSFlags & PPCII::XFormMemOp)
+    FuncInfo->setHasNonRISpills();
----------------
Just another minor suggestion, since this is probably going to be useful in other places, can you just provide something like `static bool isXFormMemOp(unsigned Opcode);` in `PPCInstrInfo`? Then we can query it wherever we need it. I don't imagine there's anything about that query that would require an actual instance of the class, but I haven't checked so it may not be possible to make it `static`.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.h:72
+  UseVSXReg = 0x1 << NewDef_Shift,
+  /// This instruction is X-Form
+  XFormMemOp = 0x1 << (NewDef_Shift+1)
----------------
`/// This instruction is an X-Form memory operation.`


https://reviews.llvm.org/D43086