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

[Andrew J Wock via Phabricator via llvm-commits]

ajwock marked 3 inline comments as done.
ajwock added a comment.

Still have some test changes that I have to make- slight diff change incoming soon



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:1577
 
+void DAGTypeLegalizer::ExpandFloatRes_STRICT_FP_EXTEND(SDNode *N, SDValue &Lo,
+                                                SDValue &Hi) {
----------------
jsji wrote:
> 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.
I didn't find it necessary to add such comments- libcallification is one of the possible ways one might expand a floating point result, but it isn't something that is implied.

Another reason I didn't do this is that the author of the analog ExpandFloatRes_FP_EXTEND (not strict and right above this) didn't comment on this, and when I came across this code in my problem solving, I could reasonably intuit why FP_EXTEND wouldn't be a libcall.

I could add documentation that explains this issue with respect to both this function and the above function, but wouldn't that be more appropriate for a ticket specifically about existing documentation?


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:1039
+
+  Call = TLI.makeLibCall_Chained(DAG, LC, N->getValueType(0), 
+                                   Ops, isSigned, 
----------------
jsji wrote:
> Can we just reuse `ExpandChainLibCall` below?
Good catch.  I wonder why this function was named so differently- that alone kept me from realizing it had anything to do with LibCallify.

Fixed, and removed LibCallifyStrictFP + makeLibCallChained.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64662