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

[Jun Sha via Phabricator via llvm-commits]

joshua-arch1 abandoned this revision.
joshua-arch1 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:428
     setLoadExtAction(ISD::EXTLOAD, MVT::f32, MVT::f16, Expand);
+    setLoadExtAction(ISD::EXTLOAD, MVT::f32, MVT::bf16, Expand);
     setTruncStoreAction(MVT::f32, MVT::f16, Expand);
----------------
craig.topper wrote:
> This wasn't needed by D151663? Is there a test case missing from D151663 that requires this?
Maybe I can remove this part. In my first version of this patch, the expand is needed for LoadExt and TruncStore.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:432
+    setOperationAction(ISD::FP_TO_BF16, MVT::f32, Custom);
+    setOperationAction(ISD::BF16_TO_FP, MVT::f32, Custom);
     setOperationAction(ISD::IS_FPCLASS, MVT::f32, Custom);
----------------
craig.topper wrote:
> D151663 uses 
> 
> ```
>     setOperationAction(ISD::FP_TO_BF16, MVT::f32,
>                        Subtarget.isSoftFPABI() ? LibCall : Custom);
> ```
> 
> Is that difference important?
Since we use also libcall in the lowerFP_TO_BF16 function, I don't think we need to use "Libcall" here for SoftABI.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2461
+  if (Op.getOperand(0).getValueType() == MVT::bf16) {
+    Op0 =
+        DAG.getNode(ISD::ANY_EXTEND, DL, XLenVT,
----------------
craig.topper wrote:
> Why wasn't this part needed in D151663?
Although bf16 type isn't legal, that does not mean this condition will never happen.


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

https://reviews.llvm.org/D151313