[PATCH] D143029: [RISCV] Add vendor-defined XTHeadBa (address-generation) extension

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

This generally looks reasonable to me, and I support adding this vendor extension once normal code review is complete.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:16
 //===----------------------------------------------------------------------===//
+
 class THInstVdotVV<bits<6> funct6, RISCVVFormat opv, dag outs, dag ins,
----------------
Stray whitespace.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:47
 
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+class THShiftALU_rri<bits<3> funct3, string opcodestr>
----------------
It looks like you're missing the decoder namerspace here.


================
Comment at: llvm/test/MC/RISCV/rv64xtheadba-valid.s:10
+# CHECK-ASM: encoding: [0x8b,0x12,0x73,0x02]
+th.addsl t0, t1, t2, 1
+# CHECK-ASM-AND-OBJ: th.addsl t0, t1, t2, 2
----------------
Is 0 a valid encoding?  If so, can you add a test covering that?

p.s. The choice to use uimm2 seems slightly odd here since it wastes one value for the shift.  Was the 0 encoding possibly used for something else?  If it was, maybe we need a different immediate type or an assert somewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143029