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

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added a comment.

Hi, thanks a lot for this.

I have some initial comments after some inital local experimentation.



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:247
+
+  struct VType {
+    VSEW sew;
----------------
You will have to replace this with another one `VType` otherwise clang (at least 7.0) will reject this code. I suggest `VTypeOp`.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:259
     struct SysRegOp SysReg;
+    struct VType VType;
   };
----------------
`VTypeOp VType;` should do here


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:665
+    switch (sew) {
+      default:
+	return "e8";
----------------
This label is unneeded because this switch is covering all of them.

As per the [[ https://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations | llvm coding standards ]] you dont't want a default in this case.

Also the formatting is unusal, make sure you run clang-format to the new code you contribute.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:687
+  static StringRef getLMULStr(VLMUL lmul) {
+    switch (lmul) {
+      default:
----------------
Ditto.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1436
+    getLexer().Lex();
+    Operands.push_back(RISCVOperand::createReg(RegNo, S, E, isRV64()));
+  }
----------------
Shouldn't we check that `RegNo == RISCV::V0` here? I tried with 

```
vadd.vv	v3, v4, v5, v0.t
vadd.vv	v3, v4, v5, v4.t
```

and I got

```
vadd.vv	v3, v4, v5, v0.t        # encoding: [0xd7,0x81,0x42,0x00]
vadd.vv	v3, v4, v5, v4.t        # encoding: [0xd7,0x81,0x42,0x00]
```

This doesn't seem right, does it?


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1927
 
+bool RISCVAsmParser::validateInstruction(MCInst &Inst,
+                                         OperandVector &Operands) {
----------------
You plan to add tests for the erros diagnosed in this function as well, right?


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1983
+          " the mask register of a different element width.");
+  }  // Fall through.
+  case RISCV::VWADDU_VV:
----------------
There is a macro `LLVM_FALLTHROUGH` for these cases. You can use it here and in the later occurrences of a similar comment.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69987