[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:164
 
+static DecodeStatus DecodeGPRF16RegisterClass(MCInst &Inst, uint64_t RegNo,
+                                              uint64_t Address,
----------------
Use `functionName`.

There is inconsistency in the code base but `functionName` is used much more than `FunctionName`.


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:186
+  if (RegNo >= 32 || RegNo & 1) {
+    return MCDisassembler::Fail;
+  }
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoD.td:175
+                                   string OpcodeStr> {
+    let Predicates = [HasStdExtD, IsRV64] in
+    def : FPUnaryOpDynFrmAlias_single<Inst, OpcodeStr, GPROp, FPR64Op>;
----------------
The indentation is inconsistent. Also applies to code above.


================
Comment at: llvm/test/MC/RISCV/rv64zhinx-valid.s:3
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+experimental-zhinx < %s \
+# RUN:     | llvm-objdump --mattr=+experimental-zhinx -M no-aliases -d -r - \
----------------
`llvm-mc` can use `%s` instead of `< %s`

I heard that not using `<` is a bit more convenient for some Windows users. (They do suffer from `llc` and `opt`'s prevailing `< %s`)


================
Comment at: llvm/test/MC/RISCV/rv64zhinx-valid.s:12
+# CHECK-ASM: encoding: [0x53,0xf5,0x22,0xc4]
+# CHECK-RV32: :[[@LINE+1]]:1: error: instruction requires the following: RV64I Base Instruction Set
+fcvt.l.h a0, t0, dyn
----------------
`[[@LINE+1]]` is a deprecated form.
Use `[[#@LINE+1]]`. Applies to many places elsewhere.


================
Comment at: llvm/test/MC/RISCV/rvzhinx-aliases-valid.s:46
+# CHECK-INST: fmadd.h a0, a1, a2, a3, dyn
+# CHECK-ALIAS: fmadd.h a0, a1, a2, a3{{[[:space:]]}}
+fmadd.h x10, x11, x12, x13
----------------
What is `{{[[:space:]]}}` used for?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93298