[PATCH] D95588: [RISCV] Implement the MC layer support of P extension

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:950
+  assert(Reg >= RISCV::X0 && Reg <= RISCV::X31 && "Invalid register");
+  if ((Reg - RISCV::X0) % 2 || Reg == RISCV::X0)
+    return false;
----------------
I think this can just be

```
return ((Reg - RISCV::X0) % 2) == 0 && Reg != RISCV::X0
```

Though the spec talks about what happens when X0_X1 is used as a source or dest, so should we really be forbidding X0?


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:443
+    if (STI.getFeatureBits()[RISCV::FeatureExtZpsfoperand]) {
+      if (!STI.getFeatureBits()[RISCV::Feature64Bit]) {
+        LLVM_DEBUG(
----------------
Fold the 64-bit check into the if above with && to reduce nesting.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:130
 def OPC_OP_FP     : RISCVOpcode<0b1010011>;
+def OPC_OP_P      : RISCVOpcode<0b1110111>;
 def OPC_OP_V      : RISCVOpcode<0b1010111>;
----------------
I think these opcodes are in numerical order, so please put this at the end to maintain that.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:131
+               opcodestr, "$rd, $rs1, $rs2, $rs3"> {
+  let Inst{31-30} = funct2;
+  let Inst{29-25} = rs3;
----------------
Are the positions correct? They don't match the 0.9.5-draft-20210617 pdf. Change log suggests it was modified in 0.5.3?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:950
+
+let EmitPriority = 2, Predicates = [HasStdExtZpn] in {
+def : InstAlias<"rdov $rd", (CSRRS  GPR:$rd, 0x009, X0)>;
----------------
Why do these need EmitPriority = 2?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:951
+let EmitPriority = 2, Predicates = [HasStdExtZpn] in {
+def : InstAlias<"rdov $rd", (CSRRS  GPR:$rd, 0x009, X0)>;
+def : InstAlias<"clrov",    (CSRRCI      X0, 0x009,  1)>;
----------------
Can we give "def : SysReg<"vxsat", 0x009>;" a name in RISCVSystemOperands.td so we can reference it's encoding directly here like we do for these aliases

```
def : InstAlias<"rdinstret $rd", (CSRRS GPR:$rd, INSTRET.Encoding, X0)>;                                                                                                                                                                                                    
def : InstAlias<"rdcycle $rd",   (CSRRS GPR:$rd, CYCLE.Encoding, X0)>;                                                                                                                                                                                                      
def : InstAlias<"rdtime $rd",    (CSRRS GPR:$rd, TIME.Encoding, X0)>;  
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95588