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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1654
+    Operands.push_back(RISCVOperand::createFPImm(F, S, isRV64()));
+  } else {
+    // Parse FP representation.
----------------
Do we need to check if the Token is AsmToken::Real?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:348
+  // We expect an 5-bit binary encoding of a floating-point constant here.
+  static uint8_t Exp_arr[31] = {
+      0b00000001, 0b01101111, 0b01110000, 0b01110111, 0b01111000, 0b01111011,
----------------
`const`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:348
+  // We expect an 5-bit binary encoding of a floating-point constant here.
+  static uint8_t Exp_arr[31] = {
+      0b00000001, 0b01101111, 0b01110000, 0b01110111, 0b01111000, 0b01111011,
----------------
craig.topper wrote:
> `const`
If you combined Exp_arr and Man_arr into an array of std::pair is it the same array as LoadFPImmArr from getLoadFPImm?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:356
+
+  static uint8_t Man_arr[31] = {
+      0b000, 0b000, 0b000, 0b000, 0b000, 0b000, 0b000, 0b000,
----------------
`const`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:376
+/// getLoadFPImm - Return an 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 5-bit` -> `as a 5-bit`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:386
+
+  static std::pair<uint8_t, uint8_t> LoadFPImmArr[] = {
+      {0b00000001, 0b000}, {0b01101111, 0b000}, {0b01110000, 0b000},
----------------
`const`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:401
+  if (Sign == 0b0) {
+    auto EMI = std::find(std::begin(LoadFPImmArr), std::end(LoadFPImmArr),
+                         std::make_pair(Exp, Mantissa));
----------------
You might be able to use `llvm::find(LoadFPImmArr)...` here.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:405
+      return std::distance(std::begin(LoadFPImmArr), EMI) + 1;
+    else
+      return -1;
----------------
No need for else since the if body returned


================
Comment at: llvm/test/MC/RISCV/rv64zfa-valid.s:457
+# CHECK-ASM: encoding: [0xd3,0x95,0x80,0xc2]
+fcvtmod.w.d a1, ft1, rtz
+
----------------
Are you enforcing that only `rtz` is valid for this instruction?


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

https://reviews.llvm.org/D140460