[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Evandro Menezes via Phabricator via llvm-commits]

evandro added a comment.

Shouldn't most instructions have the `vtype` register in `Uses`?  Shouldn't most FP instructions have `fflags` in `Defs`?

The tests are extensive, but it would be important to also add negative tests.



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:140
   OperandMatchResultTy parseJALOffset(OperandVector &Operands);
+  OperandMatchResultTy parseVTypeIAsmOperand(OperandVector &Operands);
+  OperandMatchResultTy parseMaskRegister(OperandVector &Operands);
----------------
Please, rename to `parseVTypeI()`.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:178
+
+  std::unique_ptr<RISCVOperand> defaultRVVMaskRegOp() const;
+
----------------
Please, rename to `defaultMaskRegOp()`.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:683
+    }
+    llvm_unreachable("SEW must be [8|16|32|64|128|256|512|1024]");
+  }
----------------
This should be the `default` case for the `switch` above.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:697
+    }
+    llvm_unreachable("LMUL must be [1|2|4|8]");
+  }
----------------
Ditto.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:777
+    Op->VType.sew = static_cast<VSEW>(sew.logBase2());
+    Op->VType.lmul = static_cast<VLMUL>(lmul.logBase2());
+    Op->VType.Encoding = (sew.logBase2() << 2) | lmul.logBase2();
----------------
Avoid calling `logBase2()` multiple times.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:778
+    Op->VType.lmul = static_cast<VLMUL>(lmul.logBase2());
+    Op->VType.Encoding = (sew.logBase2() << 2) | lmul.logBase2();
+    Op->StartLoc = S;
----------------
Ditto.



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1056
   }
+  case Match_InvalidVTypeIAsmOperand: {
+    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
----------------
Please, rename to `Match_InvalidVTypeI`.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1061
+  }
+  case Match_InvalidVRMaskAsmOperand: {
+    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
----------------
Please, rename to `Match_InvalidVMaskRegister`.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1386
+OperandMatchResultTy
+RISCVAsmParser::parseVTypeIAsmOperand(OperandVector &Operands) {
+  SMLoc S = getLoc();
----------------
Please, rename to `parseVTypeI()`.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1429
+OperandMatchResultTy
+RISCVAsmParser::parseMaskRegister(OperandVector &Operands) {
+  switch (getLexer().getKind()) {
----------------
Please, rename to `parseMaskReg()`.


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:166
+
+static DecodeStatus decodeVRMaskOp(MCInst &Inst, uint64_t RegNo,
+                                   uint64_t Address, const void *Decoder) {
----------------
Please, rename to `decodeVMaskReg()`.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp:163
+
+void RISCVInstPrinter::printVRMaskOp(const MCInst *MI, unsigned OpNo,
+                                     const MCSubtargetInfo &STI,
----------------
Please, rename to `printVMaskReg()`.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.h:46
+                   raw_ostream &O);
+  void printVRMaskOp(const MCInst *MI, unsigned OpNo,
+                     const MCSubtargetInfo &STI, raw_ostream &O);
----------------
Please, rename to `printVMaskReg()`.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:84
+
+  unsigned getVRMaskOp(const MCInst &MI, unsigned OpNo,
+                       SmallVectorImpl<MCFixup> &Fixups,
----------------
Please, rename to `getVMaskReg()`.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:54
+    : SubtargetFeature<"v", "HasStdExtV", "true",
+                       "'V' (Vector Instructions)">;
+def HasStdExtV : Predicate<"Subtarget->hasStdExtV()">,
----------------
Should it also imply `FeatureStdExtD`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:32
+
+def VRAsmOperand : AsmOperandClass {
+  let Name = "RVVRegOpOperand";
----------------
Please, rename to `VRegAsmOperand`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:39
+
+def VRRegOp: RegisterOperand<VR> {
+  let ParserMatchClass = VRAsmOperand;
----------------
Please, rename to `VRegOp`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:44
+
+def VRMaskAsmOperand : AsmOperandClass {
+  let Name = "RVVMaskRegOpOperand";
----------------
Please, rename to `VMaskAsmOperand`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:54
+
+def VRMaskRegOp: RegisterOperand<VRV0> {
+  let ParserMatchClass = VRMaskAsmOperand;
----------------
Please, rename to `VMaskOp`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:77
+
+let hasSideEffects = 0, mayLoad = 1, mayStore = 0 in {
+// load vd, (rs1), vm
----------------
No need to set fields to the default `0`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:534
+
+// Vector Fixed-Point Arithmetic Instructions
+defm VSADDU_V : VALU_IV_V_X_I<"vsaddu", 0b100000>;
----------------
Shouldn't these have `vxsat` in `Defs`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:540
+
+// Vector Single-Width Averaging Add and Subtract
+defm VAADDU_V : VALU_MV_V_X<"vaaddu", 0b001000>;
----------------
Shouldn't these have `vxrm` in `Uses`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:546
+
+// Vector Single-Width Fractional Multiply with Rounding and Saturation
+defm VSMUL_V : VALU_IV_V_X<"vsmul", 0b100111>;
----------------
Shouldn't this hace `vxrm` in `Uses` and `vxsat` in `Defs`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:549
+
+// Vector Quad-Widening Integer Multiply-Add Instructions
+defm VQMACCU_V : VALUr_IV_V_X<"vqmaccu", 0b111100>;
----------------
Aren't these part of the base V extension?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:555
+
+// Vector Single-Width Scaling Shift Instructions
+defm VSSRL_V : VALU_IV_V_X_I<"vssrl", 0b101010, uimm5>;
----------------
Shouldn't this hace `vxrm` in `Uses`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:559
+
+// Vector Narrowing Fixed-Point Clip Instructions
+defm VNCLIPU_W : VALU_IV_V_X_I<"vnclipu", 0b101110, uimm5, "w">;
----------------
Shouldn't this hace `vxrm` in `Uses` and `vxsat` in `Defs`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69987