[PATCH] D151313: [RISCV][BF16] Make backend type bf16 to follow the psABI

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2437
+                                             SelectionDAG &DAG) const {
+  SDLoc DL(Op);
+  SDValue Op0;
----------------
This is an incorrect conversion. We can't truncate the mantissa. It would turn some nan encodings into infinity. Probably other issues I haven't thought of yet.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2468
+                       DAG.getNode(ISD::TRUNCATE, DL, MVT::i16, Res));
+  } else {
+    return Res;
----------------
No need for else after an `if` that returns


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2485
+    Op0 =
+        DAG.getNode(ISD::ANY_EXTEND, DL, MVT::i32,
+                    DAG.getNode(ISD::BITCAST, DL, MVT::i16, Op.getOperand(0)));
----------------
Doesn't this need to be i64 on 64 bit target?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2486
+        DAG.getNode(ISD::ANY_EXTEND, DL, MVT::i32,
+                    DAG.getNode(ISD::BITCAST, DL, MVT::i16, Op.getOperand(0)));
+  } else {
----------------
This creates an MVT::i16 after type legalization which is illegal.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2492
+  if (Subtarget.is64Bit()) {
+    SDValue Shift = DAG.getNode(
+        ISD::SHL, DL, MVT::i64, Op0,
----------------
You share the shift code between both paths by using XLenVT.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2494
+        ISD::SHL, DL, MVT::i64, Op0,
+        DAG.getConstant(16, DL,
+                        TLI.getShiftAmountTy(MVT::i64, DAG.getDataLayout())));
----------------
Use getShiftAmountConstant


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2508
+    return DAG.getNode(ISD::FP_EXTEND, DL, Op.getValueType(), Res);
+  } else {
+    return Res;
----------------
No need for `else` after `if` returns


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

https://reviews.llvm.org/D151313