[PATCH] D11438: Fix x86_64 fp128 calling convention

[Chih-Hung Hsieh via llvm-commits]

chh added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8589
@@ +8588,3 @@
+  // Do not optimize out type conversion of f128 type yet.
+  // Store and truncate operators for f128 are very limited.
+  EVT N1VT = N1->getValueType(0);
----------------
davidxl wrote:
> Can you explain this a little more?
Done. Please see new comment in code.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8593
@@ -8589,1 +8592,3 @@
+  if ((N1.getOpcode() == ISD::FP_EXTEND || N1.getOpcode() == ISD::FP_ROUND) &&
+      (N1VT == N1Op0VT || (N1VT != MVT::f128 && N1Op0VT != MVT::f128)))
     return DAG.getNode(ISD::FCOPYSIGN, SDLoc(N), VT,
----------------
davidxl wrote:
> Should it be  ... && (N1VT == N1OpVT || N1OpVT != MVT::f128)?  
Good suggestion. I might be too conservative here.
Now I changed the condition to only N1Op0VT != MVT::f128,
and all my tests are still working.


================
Comment at: lib/CodeGen/SelectionDAG/InstrEmitter.cpp:169
@@ -164,1 +168,3 @@
+    // If TRI->getCommonSubClass(UseRC, RC) returns some register class,
+    // e.g. FR128, that does not contain VT, it should not be used for DstRC.
     DstRC = UseRC;
----------------
davidxl wrote:
> Why hasType(VT) returns false for FR128? Is it expected?
See also my previous answers and comments at line 143.
The question is whether ComRC returned by TRI->getCommonSubClass should guarantee ComRC->hasType(Node->getSimpleValueType(ResNo), or FR128 should be defined to work with existing getCommonSubClass. I couldn't find a clean way to change either FR128 or  getCommonSubClass, without breaking many other things. So, here it seems to me the smallest fix on the user of ComRC (UseRC).


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3535
@@ +3534,3 @@
+    ConstantSDNode *CP = cast<ConstantSDNode>(Node);
+    Results.push_back(ExpandConstant(CP));
+    break;
----------------
davidxl wrote:
> unconditionally expand? Do you need to add some assertion on the type expected?
Is there some invalid constant type?
I think ExpandConstant(CP) should already check necessary node types.



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:52
@@ -51,2 +51,3 @@
         dbgs() << "\n");
-  SDValue R = SDValue();
+  SDValue R(N, ResNo);
+
----------------
davidxl wrote:
> why this change?
The new default value R will make other code simpler like SetSoftenedFloat(R, R),
which just wants to mark this node as softened without changing it.



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:68
@@ +67,3 @@
+      case ISD::SELECT_CC:
+        return false;
+      // On x86_64 FABS, FNEG, and FCOPYSIGN can be simplified to
----------------
davidxl wrote:
> Why returning false for some cases and true for others?
See new inline comment at line 62.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:78
@@ +77,3 @@
+        // Mark this node as softened.
+        SetSoftenedFloat(R, R);
+        return true;
----------------
davidxl wrote:
> Explain this change?
Added more comments in code, line 62.
Hope that will make more sense to the comments about FABS, FCOPYSIGN, FNEG, ConstantFP.




================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:145
@@ +144,3 @@
+    // on the ScanOperands in LegalizeTypes.cpp to replace every operand.
+    // But SoftenFloatOperand is not smart enough to replace every operand.
+    // See CodeGen/X86/fp128-abs.ll, fp128-load.ll.
----------------
davidxl wrote:
> Should the problem be fixed in SoftenFloatOperand method?
SoftenFloatOperand works with ScanOperands if !KeepFloat.
So, the new condition here is when KeepFloat, we should call ReplaceValueWith.
It might be possible to fix every SoftenFloatOperand method and also simplify ScanOperands, but that looks like a more complex and risky move.




http://reviews.llvm.org/D11438