[PATCH] D15134: Part 1 to fix x86_64 fp128 calling convention.

[Chih-Hung Hsieh via llvm-commits]

chh marked 4 inline comments as done.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8776
@@ -8769,1 +8775,3 @@
+  if ((N1.getOpcode() == ISD::FP_EXTEND || N1.getOpcode() == ISD::FP_ROUND) &&
+      (N1VT == N1Op0VT || N1Op0VT != MVT::f128))
     return DAG.getNode(ISD::FCOPYSIGN, SDLoc(N), VT,
----------------
davidxl wrote:
> Move the comment and additional check into a small helper for readability:
> 
> if ( (.... || ...) && legalToSimplifyCopysign(N1))
Maybe you want something like IsOkayToConvertRoundOrExtendToCopySign(N1)?
The name legalToSimplifyCopysign(N1) here seems confusing,
as there are other 6 cases before this one to combine FABS, FNEG, and FCOPYSIGN operands
to FCOPYSIGN or FABS, FNEG, etc.
Maybe we could keep this condition close to its single user for now
until we know a clear abstraction?


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:775
@@ +774,3 @@
+  if (!isLegalInHWReg(N->getOperand(OpNo).getValueType()))
+    return false;
+  // When the operand type can be kept in registers, SoftenFloatResult
----------------
davidxl wrote:
> llvm_unreachable? When this function is called, isLegalInHWReg must be true.
Yes, but it should be better for this function to return false,
then the current caller will reuse existing dbg dump and llvm_unreachable.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.h:400
@@ +399,3 @@
+    if (!SoftenedOp.getNode() &&
+        isSimpleLegalType(Op.getValueType()))
+      return Op;
----------------
davidxl wrote:
> if (!SoftenedOp.getNode()) {
>    assert(isSimpleLegalType(...));  // or should it be assert(isLegalInHWReg(...)) ?
>    return Op;
> }
I am not sure about assert here.
Original code will assert(SoftenOp.getNode() && ...) after RemapValue(SoftenedOp);
I cannot prove any error there, so I only want to have a condition here to return Op.



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.h:414
@@ +413,3 @@
+    // by calling ReplaceValueWith here to update all users.
+    if (NewRes.getNode() != N && isLegalInHWReg(N->getValueType(ResNo)))
+      ReplaceValueWith(SDValue(N, ResNo), NewRes);
----------------
davidxl wrote:
> It is clearer to do:
> 
>   if (!isLegal...)
>       return;
> 
>   if (...) 
Would it be acceptable to test (NewRes.getNode() != N) first, as it should be faster than isLegalInHWReg and not always true.



http://reviews.llvm.org/D15134