[PATCH] D144249: [RISCV] Add vendor-defined XTheadMemIdx (Indexed Memory Operations) extension

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2224
+      Index = N.getOperand(0);
+      ShiftAmt = N.getConstantOperandVal(1);
+    } else {
----------------
Do we need to check the shift amount is 1,2, or 3?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:689
+  ValueType vt = XLenVT> {
+def : Pat<(st (vt GPR:$rd), GPR:$rs1, simm5:$off),
+          (Inst GPR:$rd, GPR:$rs1, simm5:$off, 0)>;
----------------
We might look at a ComplexPattern here to match a shifted simm5 and return the simm5 and the shift amount as 2 results.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:145
+class THLoadIndexed<RegisterClass VT, bits<5> funct5,
+  string opcodestr> : RVInstR<!shl(funct5, 2), !if(!eq(VT, GPR), 0b100, 0b110), OPC_CUSTOM_0,
+  (outs VT:$rd), (ins GPR:$rs1, GPR:$rs2, uimm2:$uimm2),
----------------
mtsamis wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > Split this line. Keep `string opcodestr>` on the first line lined up with RegisterClass on the previous line. Put the reset on a new line
> > When is VT not GPR?
> Sorry, I forgot to mention the reasoning behind these.
> I'm also developing the XTHeadFMemIdx (floating point indexed ops) extension which reuses these patterns with FPR32/64 for RegisterClass and f32/f64 for ValueType to avoid excess code duplication.
> 
> Apart from me forgetting to tell the reason, If you prefer it I can change this to only work with GPR for now and move the generic version to the FMemIdx patch.
We can leave it like this. I should have checked the spec for an FP version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144249