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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
