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

[Ferran Pallarès Roca via Phabricator via llvm-commits]

fpallares added a comment.

Hi, thank you for this patch!



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1939
+                                         OperandVector &Operands) {
+  SMLoc Loc = Operands[1]->getStartLoc();
+  const unsigned Opcode = Inst.getOpcode();
----------------
`Operands[1]` here might be an out-of-bounds access for some instructions with less than 2 operators (e.g. `RISCV::JALR`).


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1431
+    StringRef Name = getLexer().getTok().getIdentifier();
+    if (!Name.consume_back(".t"))
+      return MatchOperand_NoMatch;
----------------
There is an issue here: we are accepting an instruction like `vadd.vv v1, v2, v3, v0` (without `.t` suffix). This happens because `RISCVAsmParser::parseOperand` falls back to parse a register if a specific parsing like this function returns `MatchOperand_NoMatch`.

I've been looking into how to fix that and I believe we can diagnose an error here doing something like this:

```lang=cpp
    if (!Name.consume_back(".t")) {
      Error(getLoc(), "expected '.t' suffix");
      return MatchOperand_ParseFail;
    }
```

In summary:
- `v0.t` is accepted by the current parser, this is already OK.
- things like `v0.` or `v0.x` will be rejected by the proposed change.
- things like `v4.t` are already rejected by the `PredicateMethod` of `VRMaskAsmOperand`. I'd suggest adding a `DiagnosticType` in `VRMaskAsmOperand` in tablegen (`RISCVInstrInfoV.td`) in order to give a more precise diagnostic.

Does this make sense to you?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69987