[PATCH] D141984: [RISCV][MC] Add support for experimental zfa extension(FLI instruction not included)

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

A collection of minor comments.  I did not closely examine the binary encodings.

This review is missing changes to:
clang/test/Preprocessor/riscv-target-features.c
test/CodeGen/RISCV/attributes.ll
test/MC/RISCV/attribute-arch.s

(These are just boilerplate for the new extension name.)



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1866
 include "RISCVInstrInfoZfh.td"
+include "RISCVInstrInfoZfa.td"
 include "RISCVInstrInfoZicbo.td"
----------------
Alphabetical please.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td:58
+let Predicates = [HasStdExtZfa] in {
+def FMINM_S: FPALU_rr<0b0010100, 0b010, "fminm.s", FPR32, /*Commutable*/ 1>;
+def FMAXM_S: FPALU_rr<0b0010100, 0b011, "fmaxm.s", FPR32, /*Commutable*/ 1>;
----------------
This is a purely optional comment.

You have the instructions grouped by data type.  It would make the definitions easier to follow if you instead grouped by instruction name.  This would involve slightly more predicate scopes, but would more directly match the wording in the specification.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td:66
+def FLEQ_S : FPCmp_rr<0b1010000, 0b100, "fleq.s", FPR32, /*Commutable*/ 1>;
+} // Predicates = [HasStdExtZfa]
+
----------------
All of these are under Zfa, you can use an outer scope for this and an inner scope for the floating point extensions.  


================
Comment at: llvm/test/MC/RISCV/rv32zfa-valid.s:1
+# RUN: llvm-mc %s -triple=riscv32 -mattr=+experimental-zfa,+d,+zfh -riscv-no-aliases -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
----------------
This file should be zfa-valid.s (i.e. remove the rv32) since it tests both rv32 and rv64.  


================
Comment at: llvm/test/MC/RISCV/rv32zfa-valid.s:46
+# CHECK-ASM: encoding: [0xd3,0x14,0x49,0x40]
+fround.s fs1, fs2, rtz
+
----------------
If I'm reading the spec correctly, other rounding modes are valid on this instruction right?  If so, would you mind adding at least one other rounding mode example?  There's another instruction in this own set where only rtz is valid, and having a test which clearly distinguishes them would help readability.  


================
Comment at: llvm/test/MC/RISCV/rv32zfa-valid.s:110
+# CHECK-ASM: encoding: [0xd3,0x95,0x80,0xc2]
+fcvtmod.w.d a1, ft1, rtz
+
----------------
We need a zfa-invalid.s example here with a different rounding mode.  (Since this instruction only allows rtz).  


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

https://reviews.llvm.org/D141984