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

Jessica Clarke via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 24 02:06:09 PST 2020

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...

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list