[PATCH] D140460: [RISCV][MC] Add FLI instruction support for the experimental zfa extension

[Jun Sha via Phabricator via llvm-commits]

joshua-arch1 added a comment.

Ping.



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:499
+  /// Return true if the operand is a valid fli floating-point immediate.
+  bool isLoadFPImm() const {
+    return Kind == KindTy::FPImmediate &&
----------------
craig.topper wrote:
> I think to fix the "min" problem for double and half, you need a separate version of this function for fli.h and fli.d so that you can call getLoadFP16Imm and getLoadFP64Imm. To do that you need a different `AsmOperandClass` for each instruction.
I tried to use separate ASMOperandClass for fli.h and fli.d, but values apart from "min" went wrong. As far as I'm concerned, support for specific values of "min" needs much more extra work than expected. It may even affect other normal value which is equal for fli.s/fli.d/fli.h. That is why spec suggests using "min". I don't think there will be occasions when we need to use specific values of "min".


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:499
+  /// Return true if the operand is a valid fli floating-point immediate.
+  bool isLoadFPImm() const {
+    return Kind == KindTy::FPImmediate &&
----------------
joshua-arch1 wrote:
> craig.topper wrote:
> > I think to fix the "min" problem for double and half, you need a separate version of this function for fli.h and fli.d so that you can call getLoadFP16Imm and getLoadFP64Imm. To do that you need a different `AsmOperandClass` for each instruction.
> I tried to use separate ASMOperandClass for fli.h and fli.d, but values apart from "min" went wrong. As far as I'm concerned, support for specific values of "min" needs much more extra work than expected. It may even affect other normal value which is equal for fli.s/fli.d/fli.h. That is why spec suggests using "min". I don't think there will be occasions when we need to use specific values of "min".
When we write an assembly with a floating-point constant, it will be recognized as  single-precision by default. That is why how we address "min" and other values differ.


================
Comment at: llvm/test/MC/RISCV/zfa-valid.s:237
+# CHECK-NO-EXT: error: instruction requires the following: 'Zfa' (Additional Floating-Point){{$}}
+fli.h ft1, min
+
----------------
craig.topper wrote:
> There's no test for parsing the minimum value as an FP value.
As far as I'm concerned, support for specific values of "min" needs much more extra work than expected. It may even affect other normal value which is equal for fli.s/fli.d/fli.h. That is why spec suggests using "min". I don't think there will be occasions when we need to use specific values of "min". When we write an assembly with a floating-point constant, it will be recognized as single-precision by default. That is why how we address "min" and other values differ.


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

https://reviews.llvm.org/D140460