[PATCH] D144530: [RISCV] MC layer support for SiFive VCIX vendor extension.

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

I haven't looked through the .td description yet, but left some first comments (all pretty easily addressable and minor).



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2721
+    }
+  } else {
+    unsigned DestReg = Inst.getOperand(0).getReg();
----------------
The usual LLVM style is to use early returns to reduce indentation. see https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

Also, we already have `unsigned Opcode = Inst.getOpcode();` above, so you can just use that rather than introducing VCIXOpcode.


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:536
+                                AssemblerPredicate<(all_of FeatureVendorXSfvcp),
+                                "'XSfvcp' (SiFive custom vector coprocessor interface instructions)">;
+
----------------
Nit: We're not very consistent, but I think overall it's more common we use title case for the description in parentheses.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1903
 include "RISCVInstrInfoXTHead.td"
+include "RISCVInstrInfoXsf.td"
----------------
Nit: Shouldn't this be RISCVInstrInfoXSf to match the capitalisation you use in the RISCVUsage addition?


================
Comment at: llvm/test/MC/RISCV/rvv/xsfvcp-invalid.s:2
+# RUN: not llvm-mc -triple=riscv64 --mattr=+v,+xsfvcp %s 2>&1 \
+# RUN:        | FileCheck %s --check-prefix=CHECK-ERROR
+
----------------
We typically have rv32 CHECK lines as well as rv64


================
Comment at: llvm/test/MC/RISCV/rvv/xsfvcp.s:9
+# RUN: llvm-mc -triple=riscv64 -filetype=obj --mattr=+v,+xsfvcp %s \
+# RUN:        | llvm-objdump -d - | FileCheck %s --check-prefix=CHECK-UNKNOWN
+
----------------
We typically have rv32 CHECK lines as well as rv64


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144530