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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:76
+enum OpcodesForSpillKey {
+  PPC_STW,
+  PPC_STD,
----------------
I like the approach of having named indices into the data structure. However, I find the names a little too tied to the current opcodes. It is especially confusing when the enumerator name doesn't match the actual opcode. For example, on P9, you'll spill a VSR register using `PPC::STXV` which you get through the `OpcodesForSpillKey::PPC_STXVD2X` enumerator.

Also, it is somewhat customary and aids readability to have the name of the `enum` encoded in the names of the enumerators.

Can you rename them to something like `enum SpillOpcodeKey` for the enum and `SOK_Int4Spill`, `SOK_Float8Spill`, or something along those lines.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:356
+
+  for (unsigned i = 0; i < PPC_LAST_OPCODE_SPILL; i++) {
+    if (OpcodesForSpill[i] == Opcode) {
----------------
Not that this makes a very big difference, but it is a little more obvious what is happening if you just use `std::find` here.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1108
+
+  if (Opcode == PPC::SPILL_CR || Opcode == PPC::SPILL_CRBIT)
+    FuncInfo->setSpillsCR();
----------------
I would remove the reliance on opcodes here. You have the register class. Check it and call `setSpillsCr()` and `setSpillsVRSAVE()` accordingly.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1114
+
+  if (Opcode == PPC::STVX || Opcode == PPC::STXV || Opcode == PPC::STXVD2X ||
+      Opcode == PPC::DFSTOREf64 || Opcode == PPC::STXSDX ||
----------------
We are once again keeping a list of opcodes separately from the function that is supposed to have the definitive list of spill opcodes. So we aren't much better off.
I think would be a much cleaner approach to just use a query for whether an opcode is an X-Form or not. A similar trick is used in `PPCRegisterInfo` with the use of `ImmToIdxMap`.

I think the most reliable solution would be to implement this in the .td files with a mapping (similar to `RecFormRel`), but that can be left as a refactoring exercise for the future. For now, I'd implement a set of static function where one will contain the `ImmToIdxMap` and the rest will just perform queries on it:
```
static bool PPC::isXFormLdSt(unsigned Opcode);
static bool PPC::isDFormLdSt(unsigned Opcode);
static unsigned PPC::getXFormForDForm(unsigned Opcode);
static unsigned PPC::getDFormForXForm(unsigned Opcode);
```

Finally, in this function, I would assert that `isXFormLdSt(Opcode) || isDFormLdSt(Opcode)` to ensure that if we add another opcode for spilling/restoring, we don't break things.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.h:129
                                  bool &SeenIntermediateUse) const;
+  const unsigned *getOpcodesForSpillArray(bool ArchPower9) const;
   virtual void anchor();
----------------
This is a member function and you have a `PPCSubtarget` data member. You can check whether you're compiling for Power9 there, can't you?


https://reviews.llvm.org/D43086