[PATCH] D88044: [llvm-exegesis][PowerPC] Add more register classes

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I think the code being added here (and perhaps the existing code as well) requires deep expertise in both PowerPC ISA and Exegesis. But I think with more descriptive comments, we can make it consumable for those that only check one of those boxes. So most of my comments are requests to add more descriptive/explanatory comments.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstr64Bit.td:39
   let DecoderMethod = "decodeSImmOperand<16>";
+  let OperandType = "OPERAND_IMMEDIATE";
 }
----------------
I assume you have tracked down other definitions for `immediate` nodes that don't have this set and there are no others?


================
Comment at: llvm/tools/llvm-exegesis/lib/PowerPC/Target.cpp:75
+  if (IT.getInstr().hasTiedRegisters())
+    MemOpIdx = 1;
+  int DispOpIdx = MemOpIdx + 1;
----------------
If the instruction is a store, won't the value being stored be one of the operands?


================
Comment at: llvm/tools/llvm-exegesis/lib/PowerPC/Target.cpp:79
+  if (DispOp.isReg())
+    // Hardcoded X1 for X-form Memory Operations
+    setMemOp(IT, DispOpIdx, MCOperand::createReg(PPC::X1));
----------------
Because the stack pointer is guaranteed to point to accessible memory? I still don't understand why we wouldn't materialize the offset into a register here.
It seems odd that for D-Forms, we will access memory `Reg + Offset` and for X-Forms, we will access memory `Reg + <Stack>`. Seems this is something that should be documented.


================
Comment at: llvm/tools/llvm-exegesis/lib/PowerPC/Target.cpp:94
+  if (PPC::F4RCRegClass.contains(Reg))
+    return {loadImmediate(PPC::X11, 64, Value),
+            MCInstBuilder(PPC::MTVSRD).addReg(Reg).addReg(PPC::X11)};
----------------
Two concerns with this:
1. I assume you are using X11 as it is the last caller saved reg in the allocation order and therefor least likely to be used. Can you document this (along with why you think it is safe to use)? Also, can you give it a descriptive name?
2. There does not appear to be any checking that the immediate fits into a 16-bit signed field. Are we guaranteed to get small immediates? Or perhaps we don't really care that the value we get here is the actual value in the register? Please add a comment.


================
Comment at: llvm/tools/llvm-exegesis/lib/PowerPC/Target.cpp:100
+  if (PPC::VSRCRegClass.contains(Reg))
+    return {loadImmediate(PPC::X11, 64, Value), MCInstBuilder(PPC::MTVSRDD)
+                                                    .addReg(Reg)
----------------
This seems highly suspicious and should be documented if it is correct. For VR registers, we populate a single doubleword. But for VSR registers we populate both doublewords? Why? Also, is there a check elsewhere for P9Vector since you're using MTVSRDD which was added in ISA 3.0?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88044