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

[Manolis Tsamis via Phabricator via llvm-commits]

mtsamis marked 14 inline comments as done.
mtsamis added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:144
+  hasSideEffects = 0, mayLoad = 1, mayStore = 0 in {
+class THLoadIndexed<RegisterClass VT, bits<5> funct5,
+  string opcodestr> : RVInstR<!shl(funct5, 2), !if(!eq(VT, GPR), 0b100, 0b110), OPC_CUSTOM_0,
----------------
craig.topper wrote:
> I would calling RegisterClass `VT` since there's all ValueType class that would typically be associated with VT. Looks like we use `Ty` or `RegTy` or some other variation involving "Ty" or "ty".
Indeed, changed to Ty


================
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),
----------------
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.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:598
+          (Inst GPR:$rs1, GPR:$rs2, uimm2:$uimm2)>;
+let AddedComplexity = 1 in
+def : Pat<(vt (LoadOp (AddrRegZextRegScale GPR:$rs1, GPR:$rs2, uimm2:$uimm2))),
----------------
craig.topper wrote:
> Can we avoid AddedComplexity here by using the complexity parameter in the ComplexPattern?
Done, but for some reason I had to set the Complexity pararameter to 10 in order to achieve the same effect (i.e. the zext pattern being preferred from the plain one).
I don't know why it is different from AddedComplexity.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:619
+defm : LdIdxPat<zextloadi16, TH_LRHU, TH_LURHU>;
+defm : LdIdxPat<extloadi32, TH_LRW, TH_LURW, i64>;
+defm : LdIdxPat<sextloadi32, TH_LRW, TH_LURW, i64>;
----------------
craig.topper wrote:
> These last 2 should be IsRV64 I think?
Thanks for spotting these. While trying to refactor the ZEXT and normal patterns into a single multiclass I completely messed these up.

I have re-wrote them all to be correct now (including RV32/64 as per your comments) and also removed the unnecessarry passing of the i32/i64 arguments since these match XLenVT.
In order to work correctly, the new code has separate LdIdxPat, LdZextIdxPat, StIdxPat, StZextIdxPat multiclasses (since the zext index is only meaningfull on RV64).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144249