[PATCH] D94579: [RISCV] add the MC layer support of P extension

Jessica Clarke via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 12 20:08:21 PST 2021


jrtc27 added a comment.

Please make sure there are tests for invalid instructions (especially checking you have the immediate ranges and predicates correct).

I have not looked at the draft P spec so these are all shallow comments from glancing over the diff.



================
Comment at: llvm/lib/Target/RISCV/RISCV.td:188
+                  AssemblerPredicate<(all_of FeatureStdExtP),
+                  "'P' (SIMD extension instructions)">;
+
----------------
"SIMD Instructions" would be most consistent with what's already here (though the V-related extensions have names that aren't so consistent with that) IMO, though the P working group should probably come up with a better name than SIMD in order to distinguish themselves from V (with P being traditional vectorisation and V being new-fangled scalable vectorisation).


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:138
 def OPC_OP_V      : RISCVOpcode<0b1010111>;
+def OPC_OP_P      : RISCVOpcode<0b1111111>;
 def OPC_BRANCH    : RISCVOpcode<0b1100011>;
----------------
Aren't all major opcodes of the form 0bXX11111 reserved for >32-bit encodings (with 0b1111111 in particular being >=80-bit)? This is Table 24.1 Chapter 24 of v20191214, and that's still present in the latest LaTeX source in the riscv-isa-manual repo.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1263
 include "RISCVInstrInfoZfh.td"
+include "RISCVInstrInfoP.td"
----------------
P comes between B and V in the canonical extension order


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:391
+
+def BITREVI_32  : ALU_P_ri5<0b1110100, 0b000, "bitrevi">;
+
----------------
If this is the same situation as the base instruction set's bit shift instructions use a uimmlog2xlen like those rather than having two different instructions (and sets of patterns)


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:395
+
+def SRAI_u_32  : ALU_P_ri5<0b1101010, 0b001, "srai.u">;
+
----------------
Capital U


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94579



More information about the llvm-commits mailing list