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

[Luís Marques via Phabricator via llvm-commits]

luismarques added a comment.

This patch is pretty big, so I haven't yet been able to delve as deeply as I wanted to, but so far I didn't notice any major issues.
Thanks to Craig for providing the initial round of feedback, and pointing out several issues.
Some nitpicking remarks:

- Several tests should probably use a hard-float ABI, to cut down on the fmvs.
- For files that handle F, D and half, it would be nice to try to be more consistent regarding the order in which the code and declarations for those appears. (When handling early returns, short-circuit evaluation, and so on, it might arguably still make sense to be inconsistent and order things based on expected frequency, as that might have a very slightly compile-time performance impact)



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2829
   // use the ABI names in register constraint lists.
-  if (Subtarget.hasStdExtF() || Subtarget.hasStdExtD()) {
+  if (Subtarget.hasStdExtZfh() || Subtarget.hasStdExtF() ||
+      Subtarget.hasStdExtD()) {
----------------
Presumably we don't need the test for `hasStdExtZfh`, since `hasStdExtF` would match anyway.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:55-58
+  FMV_H_X_RV32,
+  FMV_H_X_RV64,
+  FMV_X_ANYEXTH_RV32,
+  FMV_X_ANYEXTH_RV64
----------------
Nit: move this to before the `READ_CYCLE_WIDE`, so that it's logically closer to the other move nodes. Tweak the comment to generalize it, etc.


================
Comment at: llvm/test/CodeGen/RISCV/copysign-casts.ll:12-17
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs -mattr=+f -mattr=+d \
+; RUN:   -mattr=+experimental-zfh -target-abi ilp32d < %s \
+; RUN:   | FileCheck %s -check-prefix=RV32IFDZfh
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs -mattr=+f -mattr=+d \
+; RUN:   -mattr=+experimental-zfh -target-abi lp64d < %s \
+; RUN:   | FileCheck %s -check-prefix=RV64IFDZfh
----------------
If we have both RV32IF and RV32IFD, I wonder if we should also have RV32IFZfh, instead of only RV32IFDZfh.


================
Comment at: llvm/test/CodeGen/RISCV/half-arith.ll:2-5
+; RUN: llc -mtriple=riscv32 -mattr=+experimental-zfh -verify-machineinstrs < %s \
+; RUN:   | FileCheck -check-prefix=RV32IZFH %s
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-zfh -verify-machineinstrs < %s \
+; RUN:   | FileCheck -check-prefix=RV64IZFH %s
----------------
This test should probably use a hard-float ABI, to cut down on the fmvs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90738