[PATCH] D90738: [RISCV] Support Zfh half-precision floating-point extension.

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:990
 
+static MCRegister convertFPR64ToFPR16(MCRegister Reg) {
+  assert(Reg >= RISCV::F0_D && Reg <= RISCV::F31_D && "Invalid register");
----------------
Nit: I'd put FPR16 before FPR32 in this file (here and below).


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1251-1252
   RegNo = MatchRegisterName(Name);
   // The 32- and 64-bit FPRs have the same asm name. Check that the initial
   // match always matches the 64-bit variant, and not the 32-bit one.
   assert(!(RegNo >= RISCV::F0_F && RegNo <= RISCV::F31_F));
----------------
Comment needs updating.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1838-1843
+  // UseGPRForF16 if targeting one of the soft-float ABIs, if passing a
+  // variadic argument, or if no F16 argument registers are available.
+  bool UseGPRForF16 = true;
   // UseGPRForF32 if targeting one of the soft-float ABIs, if passing a
   // variadic argument, or if no F32 argument registers are available.
   bool UseGPRForF32 = true;
----------------
These two are always the same value (unless you want an RV16I!), just rename the F32 one to encompass F16.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1867-1872
+  if (State.getFirstUnallocated(ArgFPR16s) == array_lengthof(ArgFPR16s))
+    UseGPRForF16 = true;
   if (State.getFirstUnallocated(ArgFPR32s) == array_lengthof(ArgFPR32s))
     UseGPRForF32 = true;
   if (State.getFirstUnallocated(ArgFPR64s) == array_lengthof(ArgFPR64s))
     UseGPRForF64 = true;
----------------
Hm, this code is a bit silly, we only need to look at one of the arrays as the registers all alias each other.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1999-2001
+  // When an f16, f32 or f64 is passed on the stack, no bit-conversion is
+  // needed.
+  if (ValVT == MVT::f16 || ValVT == MVT::f32 || ValVT == MVT::f64) {
----------------
Since this code is not already ISA/ABI-dependent this should be fine?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:33-35
 // Because RISCVReg64 register have AsmName and AltNames that alias with their
 // 32-bit sub-register, RISCVAsmParser will need to coerce a register number
 // from a RISCVReg32 to the equivalent RISCVReg64 when appropriate.
----------------
Needs updating to mention RISCVReg16? Though this is currently backwards; the 64-bit register is the canonical one given the diff in RISCVAsmParser...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90738