[PATCH] D64662: [FPEnv] [PowerPC] Lower ppc_fp128 StrictFP Nodes to libcalls

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1577
 
+void DAGTypeLegalizer::ExpandFloatRes_STRICT_FP_EXTEND(SDNode *N, SDValue &Lo,
+                                                SDValue &Hi) {
----------------
Add comments about why this is special here, not lowering to calls. I know you have description in summary,
but it would be great to add comment here, so that readers don't have to look into commit message or patches.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1913
+  case ISD::FP_ROUND:         Res = ExpandFloatOp_FP_ROUND(N); break;
+  case ISD::STRICT_FP_ROUND:  Res = ExpandFloatOp_STRICT_FP_ROUND(N); break;
+  case ISD::FP_TO_SINT:       Res = ExpandFloatOp_FP_TO_SINT(N); break;
----------------
Any reason that we are expanding `STRICT_FP_ROUND ` in `ExpandFloatOperand`, while all the others, especially `STRICT_FP_EXTEND` in `ExpandFloatResult`?


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:2013
 
+SDValue DAGTypeLegalizer::ExpandFloatOp_STRICT_FP_ROUND(SDNode *N) {
+  assert(N->getOperand(1).getValueType() == MVT::ppcf128 &&
----------------
Add comments about why this is special here, not lowering to calls too.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:1039
+
+  Call = TLI.makeLibCall_Chained(DAG, LC, N->getValueType(0), 
+                                   Ops, isSigned, 
----------------
Can we just reuse `ExpandChainLibCall` below?


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:162
+                            RTLIB::Libcall LC, EVT RetVT,
+                            ArrayRef<SDValue> Ops, bool isSigned,
+                            const SDLoc &dl, bool doesNotReturn,
----------------
If we are going to keep this API, should use `MakeLibCallOptions` introduced in https://reviews.llvm.org/D65795 instead.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:181
+    SDValue Op = *OpIterator;
+    Entry.Node = Op;
+    Entry.Ty = Entry.Node.getValueType().getTypeForEVT(*DAG.getContext());
----------------
Most of these code are command and `copied` from `makeLibCall`, can we maintain one copy only?


================
Comment at: test/CodeGen/PowerPC/ppcf128-constrained-fp-instrinsics.ll:1974
+
+define ppc_fp128 @test_fpext_ppc_fp128_f64(double %first) {
+; PC64LE-LABEL: test_fpext_ppc_fp128_f64:
----------------
These tests are all simple test with one call. Can we add one or more complex tests? eg: one call use the other calls res? so that we can test the `input` chain?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64662