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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1580
+  // Handle negation, as that still comes through as a separate token.
+  bool isNegative = parseOptionalToken(AsmToken::Minus);
+
----------------
Capitalize `isNegative`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:372
+namespace RISCVLoadFPImm {
+inline static unsigned getFPImm(unsigned Imm) {
+  uint8_t Sign;
----------------
Use uint32_t for the result to make it explicitly 32 bits


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:391
+/// getLoadFP32Imm - Return a 5-bit binary encoding of the 32-bit
+/// floating-point immediate value. If the value cannot be represented as an
+/// 5-bit binary encoding, then return -1.
----------------
"as an" -> "as a"


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:394
+static inline int getLoadFP32Imm(const APInt &Imm) {
+  uint8_t Sign = Imm.lshr(31).getZExtValue() & 1;
+  uint8_t Exp = Imm.lshr(23).getZExtValue() & 0xff;
----------------
You can use `Imm.extractBitsAsZExtValue` to extract the fields.



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:408
+static inline int getLoadFP64Imm(const APInt &Imm) {
+  uint8_t Sign = Imm.lshr(63).getZExtValue() & 1;
+  uint8_t Mantissa = Imm.lshr(49).getZExtValue() & 0x7;
----------------
Use `extractBitsAsZExtValue`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp:159
+  const MCOperand &MO = MI->getOperand(OpNo);
+  if (MO.getImm() == 1)
+    O << "min";
----------------
I'd prefer we print "inf" and "nan" explicitly here and not rely on raw_ostream's printer. Especially for infinity since "INF" is capitalized while "min" and "nan" are lower case.


================
Comment at: llvm/test/MC/RISCV/rv32zfa-valid.s:154
+# CHECK-ASM: encoding: [0xd3,0x80,0x10,0xf2]
+fli.d ft1, min
+
----------------
no test for the minimum written as an FP literal. Does the code accept the correct value or does it accept the single precision minimum value for the double precision instruction?


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

https://reviews.llvm.org/D140460