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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


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


================
Comment at: llvm/tools/llvm-exegesis/lib/PowerPC/Target.cpp:75
+  if (IT.getInstr().hasTiedRegisters())
+    MemOpIdx = 1;
+  int DispOpIdx = MemOpIdx + 1;
----------------
nemanjai wrote:
> If the instruction is a store, won't the value being stored be one of the operands?
No. `Value` operand is not included.
This is for `MemoryOperands` only, which is actually only the addressing operand in real llvm `MI`..


================
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));
----------------
nemanjai wrote:
> 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.
I will add comment. No, we don't really care about the address for now. 


================
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)};
----------------
nemanjai wrote:
> 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.
Sure.
Yeah, we don't really care the actual value in register.
This is to form a valid MI, and repeat it for certain times, to measure latency/throughput etc, we don't really care about the values at all.



================
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)
----------------
nemanjai wrote:
> 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?
As I mentioned above, we don't really care about the values in reg. We just want to generate a sequence of valid assembly..

And this will generate a code in a snippet , did not check cpu at all for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88044