[PATCH] D141560: [RISCV][CodeGen] Add codegen pattern for experimental zfa extension

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1487
+
   return Imm.isZero();
 }
----------------
I think we can't reach this line when Zfa is enabled now? getLoadFP32Imm/getLoadFP64Imm/getLoadFP16Imm return -1 for 0.0 and -0.0 right?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2004
+      if (SatVT == DstVT)
+        Opc = IsSigned ? RISCVISD::FCVTMOD_X : RISCVISD::FCVTMOD_XU;
+      else if (DstVT == MVT::i64 && SatVT == MVT::i32)
----------------
FCVTMOD does not saturate. It returns the lower bits of the overflowed value.  These instruction cannot be used to lower saturating conversion.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:3813
     }
+    if (VT == MVT::f64 && Op0VT == MVT::i64 && XLenVT == MVT::i32) {
+      SDValue Lo = DAG.getNode(ISD::EXTRACT_ELEMENT, DL, MVT::i32, Op0,
----------------
Does this need to be qualified with Zfa?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8030
       Results.push_back(DAG.getNode(ISD::TRUNCATE, DL, MVT::i32, FPConv));
+    } else if (VT == MVT::i64 && Op0VT == MVT::f64 && XLenVT == MVT::i32) {
+        SDValue NewReg = DAG.getNode(RISCVISD::SplitF64, DL,
----------------
Does this need to be qualified with Zfa?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:114
   FCVT_WU_RV64,
+  // FP to XLen int conversions. Corresponds to fcvt.l(u).s/d/h on RV64 and
+  // fcvt.w(u).s/d/h on RV32. Unlike FP_TO_S/UINT these saturate out of
----------------
This description is incorrect. The FCVTMOD instructions do not saturate.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td:152
+  let isCodeGenOnly = 1, mayRaiseFPException = 0 in {
+    def FMVH_X_W : FPUnaryOp_r<0b1110000, 0b00000, 0b000, GPR, FPR64, "fmv.x.w">,
+            Sched<[WriteFMovF32ToI32, ReadFMovF32ToI32]>;
----------------
This name is misleading. There is no fmvh.x.w instruction. FMV_X_W_FPR64 would be better.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td:191
+def : Pat<(f32 fpimm:$imm),
+  (COPY_TO_REGCLASS (FLI_S (bitcast_fp32imm_to_loadfpimm fpimm:$imm)), FPR32)>;
+
----------------
Why do you need a COPY_TO_REGCLASS?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td:208
+
+def: PatSetCC<FPR32, strict_fsetcc, SETLT, FLTQ_S>;
+def: PatSetCC<FPR32, strict_fsetcc, SETOLT, FLTQ_S>;
----------------
What is giving these patterns priority over the ones in RISCVInstrInfoF.td?


================
Comment at: llvm/test/CodeGen/RISCV/double-zfa.ll:108
+; RV64DZFA-NEXT:    ret
+  ret double 0x102F3E9DF9CF94
+}
----------------
This doesn't look like the minimum value for normal value for double. I would expect the mantissa bits to be 0.

So it should be 0x0010000000000000 I think?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-bitcast.ll:422
 ; RV32ELEN32-NEXT:    sw a0, 8(sp)
+; RV32ELEN32-NEXT:    sw a1, 12(sp)
 ; RV32ELEN32-NEXT:    fld fa0, 8(sp)
----------------
Why does this test change? It doesn't use Zfa.


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

https://reviews.llvm.org/D141560