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

[Andrew J Wock via Phabricator via llvm-commits]

ajwock marked 9 inline comments as done.
ajwock added inline comments.


================
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, 
----------------
kbarton wrote:
> 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. 
Fixed.  This was matching how I added strict to other names, but it is not necessary.


================
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;
----------------
kbarton wrote:
> 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. 
So, each of these functions capitalized names corresponds to an instruction opcode.  In order to stay consistent with the rest of these functions, I have simply named these functions after their opcode rather than arbitrarily prepending _STRICT.

I believe that if I did not name these functions as such, it would either be inconsistent with existent code or require significant refactoring.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1242
+                                              SDValue &Hi) {
+  SDValue Call = LibCallify_StrictFP(GetFPLibCall(N->getValueType(0),
+                                         RTLIB::FMIN_F32, RTLIB::FMIN_F64,
----------------
kbarton wrote:
> Was this formatted with clang-format? 
Fixed?  Reformatted 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");
----------------
kbarton wrote:
> Can you put parenthesis around this to make it clear the order of evaluation between the && and ||?
Fixed, and switched order of evaluation to short circuit on the common case.


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


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


================
Comment at: test/CodeGen/PowerPC/ppcf128-constrained-fp-instrinsics.ll:4
+; RUN: llc -O3 -mtriple=powerpc64le-linux-gnu -mcpu=pwr9 < %s | FileCheck --check-prefix=PC64LE9 %s
+; RUN: llc -O3 -mtriple=powerpc-linux-gnu < %s | FileCheck --check-prefix=PC %s
+
----------------
kbarton wrote:
> I also just realized we don't have any 32-bit build bots anymore. 
> Can you please change this to powerpc64-linux-gnu?
Fixed.


================
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
----------------
kbarton wrote:
> 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. 
Semi-fixed:  They were- though as far as I've seen these checks are typically placed right below the function header.  I can change this if needed.


================
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
----------------
kbarton wrote:
> 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. 
Alright- loosened -NEXT restrictions around std/u instructions.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64662