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

[Jun Sha via Phabricator via llvm-commits]

joshua-arch1 marked 5 inline comments as done.
joshua-arch1 added a comment.

Ping.



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:302
+    unsigned Val;
+    bool IsRV64;
+  };
----------------
craig.topper wrote:
> Is this IsRV64 field used ever?
It isn't used yet. I added it just in order to keep consistent with ImmOp.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:409
+  uint8_t Sign = Imm.extractBitsAsZExtValue(1, 63);
+  uint8_t Mantissa = Imm.extractBitsAsZExtValue(3, 49);
+
----------------
craig.topper wrote:
> You can't use only 3 bits of the mantissa to match an immediate for `isFPImmLegal` in the other patch. It has to be bit exact for the entire 64 bit value.
In the encoding table, only the top 3 bit of the mantissa differs. We don't need to use all the bits to distinguish between different values. 3-bit expression will not influence precision.


================
Comment at: llvm/test/MC/RISCV/zfa-valid.s:197
+# CHECK-NO-EXT: error: instruction requires the following: 'Zfa' (Additional Floating-Point){{$}}
+fli.d ft1, 1.1754943508222875079687365372222456778186655567720875215087517062784172594547271728515625e-38
+
----------------
craig.topper wrote:
> This isn't the correct min value for double precision
I think we need to keep consistent with GNU. https://github.com/riscv/riscv-isa-manual/issues/980
GNU prefers to accept 'min' instead of specific values since these values are difficult to express.


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

https://reviews.llvm.org/D140460