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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

I thought I'd gone through it thoroughly last time but apparently not :( oh well hopefully the last set of nits.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:519
+      return FPConv;
+    } else if (Op.getValueType() == MVT::f16 && Subtarget.hasStdExtZfh()) {
+      if (Op0.getValueType() != MVT::i16)
----------------
Please also sort this one.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1111
+      Results.push_back(DAG.getNode(ISD::TRUNCATE, DL, MVT::i32, FPConv));
+    } else if (N->getValueType(0) == MVT::i16 && Subtarget.hasStdExtZfh()) {
+      if (Op0.getValueType() != MVT::f16)
----------------
Ditto.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2175
   case CCValAssign::BCvt:
-    if (VA.getLocVT() == MVT::i64 && VA.getValVT() == MVT::f32) {
+    if (VA.getLocVT() == MVT::i64 && VA.getValVT() == MVT::f32)
       Val = DAG.getNode(RISCVISD::FMV_W_X_RV64, DL, MVT::f32, Val);
----------------
Needs sorting too.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:53-59
+  // FPR32/FPR16<->GPR transfer operations for RV64/RV32. Needed as an i32<->f32
+  // and i16<->f16 bitcast are not legal on RV64/RV32. FMV_W_X_RV64 matches the
+  // semantics of the FMV.W.X. FMV_X_ANYEXTW_RV64 is similar to FMV.X.W but has
+  // an any-extended result. FMV_H_X_RV32/RV64 match the semantics of the
+  // FMV.H.X. FMV_X_ANYEXTH_RV32/RV64  are similar to FMV.X.H but has an
+  // any-extended result. This is a more convenient semantic for producing
+  // dagcombines that remove unnecessary GPR->FPR->GPR moves.
----------------
Comment should be updated to reflect the sorted order. It's also unclear which combinations are invalid; currently it could be interpreted as f16 only being an issue on RV32.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90738