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

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai marked 2 inline comments as done.
HsiangKai added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1939
+                                         OperandVector &Operands) {
+  SMLoc Loc = Operands[1]->getStartLoc();
+  const unsigned Opcode = Inst.getOpcode();
----------------
fpallares wrote:
> `Operands[1]` here might be an out-of-bounds access for some instructions with less than 2 operators (e.g. `RISCV::JALR`).
Operands[0] in OperandVector will be operator. Operands[1] will be the first operand.
If there is an instruction with no operand, it might be an out-of-bounds access.

I will change the default location to point to operator. Thanks.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1431
+    StringRef Name = getLexer().getTok().getIdentifier();
+    if (!Name.consume_back(".t"))
+      return MatchOperand_NoMatch;
----------------
fpallares wrote:
> 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?
It makes sense. I will modify the patch according to your comments. Thanks a lot.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69987