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

[Kit Barton via Phabricator via llvm-commits]

kbarton requested changes to this revision.
kbarton added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:2969
+  /// This function also chains to the first operand in Ops.
+  std::pair<SDValue, SDValue> makeLibCall_Chained(
+      SelectionDAG &DAG, RTLIB::Libcall LC, EVT RetVT, 
----------------
Is there a specific reason you used the _ in the function name? 
I think with camel case this can be makeLibCallChained, which is still easy to read and follows the coding guidelines. 


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1140
+  case ISD::FMINNUM:            ExpandFloatRes_FMINNUM(N, Lo, Hi); break;
+  case ISD::STRICT_FMINNUM:     ExpandFloatRes_STRICT_FMINNUM(N, Lo, Hi); break;
+  case ISD::FMAXNUM:            ExpandFloatRes_FMAXNUM(N, Lo, Hi); break;
----------------
Similar comment here with the choice of _STRICT_ vs Strict_ (i.e., ExpandFloatResStrict_FMINNUM(...)).
I think the use of Strict_ follows the coding guidelines, while _STRICT_ deviates from it. 


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1242
+                                              SDValue &Hi) {
+  SDValue Call = LibCallify_StrictFP(GetFPLibCall(N->getValueType(0),
+                                         RTLIB::FMIN_F32, RTLIB::FMIN_F64,
----------------
Was this formatted with clang-format? 


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1935
+  assert(Res.getValueType() == N->getValueType(0) && 
+         N->isStrictFPOpcode() || N->getNumValues() == 1 &&
          "Invalid operand expansion");
----------------
Can you put parenthesis around this to make it clear the order of evaluation between the && and ||?


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:1029
+//nodes in the expansion use the original incoming and outgoing chain
+//from the StrictFP Node.
+SDValue DAGTypeLegalizer::LibCallify_StrictFP(RTLIB::Libcall LC, SDNode *N,
----------------
Can you please make this into a doxygen comment .


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:1031
+SDValue DAGTypeLegalizer::LibCallify_StrictFP(RTLIB::Libcall LC, SDNode *N,
+                                              bool isSigned) {
+  unsigned NumOps = N->getNumOperands();
----------------
Same comment about the use of underscores. I think this should be LibCallifyStrictFP. 


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:160
+std::pair<SDValue, SDValue>
+TargetLowering::makeLibCall_Chained(SelectionDAG &DAG,
+                            RTLIB::Libcall LC, EVT RetVT,
----------------
This should be makeLibCallChained


================
Comment at: test/CodeGen/PowerPC/ppcf128-constrained-fp-instrinsics.ll:7
+define ppc_fp128 @test_fadd_ppc_fp128(ppc_fp128 %first,
+; PC64LE-LABEL: test_fadd_ppc_fp128:
+; PC64LE:       # %bb.0: # %entry
----------------
Were these checks automatically generated by the python script?
It seems unusual that they are placed in the middle of the parameter list for the test case. I thought they usually got put immediately before the function. 


================
Comment at: test/CodeGen/PowerPC/ppcf128-constrained-fp-instrinsics.ll:12
+; PC64LE-NEXT:    stdu 1, -32(1)
+; PC64LE-NEXT:    .cfi_def_cfa_offset 32
+; PC64LE-NEXT:    .cfi_offset lr, 16
----------------
Others may disagree with me, but I would recommend getting rid of the checks for cfi here.
I don't think the location of the cfi has any bearing on the correctness here. I also think the compiler should be free to move them with respect to the  std and stdu instructions. If it does so, this test case will fail. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64662