[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/Support/RISCVISAInfo.cpp:117
     {"xtheadmempair", RISCVExtensionVersion{1, 0}},
+    {"xtheadmemidx", RISCVExtensionVersion{1, 0}},
     {"xtheadvdot", RISCVExtensionVersion{1, 0}},
----------------
alphabetize above mempair


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:539
     }
+    if (STI.getFeatureBits()[RISCV::FeatureVendorXTHeadMemIdx]) {
+      LLVM_DEBUG(dbgs() << "Trying XTHeadMemIdx custom opcode table:\n");
----------------
Use `STI.hasFeature(RISCV::FeatureVendorXTHeadMemIdx)`. I just rewrote all the existing code to that form.


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:501
 
+def FeatureVendorXTHeadMemIdx
+    : SubtargetFeature<"xtheadmemidx", "HasVendorXTHeadMemIdx", "true",
----------------
alphbetize above MemPair


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


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


================
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:
> 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?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:154
+  : RVInstI<0b100, OPC_CUSTOM_0, (outs GPR:$rd, GPR:$rs1_wb),
+  (ins GPR:$rs1, simm5:$simm5, uimm2:$uimm2),
+  opcodestr, "$rd, (${rs1}), $simm5, $uimm2"> {
----------------
Line this up with 0b100 on the previous line


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:260
+let Predicates = [HasVendorXTHeadMemIdx] in {
+// THead Load/Store + Update instructions.
+def TH_LBIA : THLoadUpdate<0b00011, "th.lbia">,
----------------
Use the hyphenated spelling of T-Head in the comments.


================
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))),
----------------
Can we avoid AddedComplexity here by using the complexity parameter in the ComplexPattern?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:604
+multiclass StIdxPat<PatFrag StoreOp, RVInst Inst, RVInst InstZext, RegisterClass StTy,
+                 ValueType vt = XLenVT> {
+def : Pat<(StoreOp (vt StTy:$rd), (AddrRegRegScale GPR:$rs1, GPR:$rs2, uimm2:$uimm2)),
----------------
Indentation


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:605
+                 ValueType vt = XLenVT> {
+def : Pat<(StoreOp (vt StTy:$rd), (AddrRegRegScale GPR:$rs1, GPR:$rs2, uimm2:$uimm2)),
+          (Inst StTy:$rd, GPR:$rs1, GPR:$rs2, uimm2:$uimm2)>;
----------------
80 columns


================
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>;
----------------
These last 2 should be IsRV64 I think?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:624
+defm : StIdxPat<truncstorei16, TH_SRH, TH_SURH, GPR>;
+defm : StIdxPat<truncstorei32, TH_SRW, TH_SURW, GPR, i64>;
+}
----------------
This one should be IsRV64 only I think?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:629
+defm : LdIdxPat<zextloadi32, TH_LRWU, TH_LURWU, i64>;
+defm : LdIdxPat<load, TH_LRD, TH_LURD, i64>;
+
----------------
Is there a plain load pattern for TH_LRW for RV32?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:678
+defm : StoreUpdatePat<pre_truncsti16, TH_SHIB>;
+defm : StoreUpdatePat<post_store, TH_SWIA, i32>;
+defm : StoreUpdatePat<pre_store, TH_SWIB, i32>;
----------------
These last 2 patterns should be IsRV32 only I think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144249