[PATCH] D11438: Fix x86_64 fp128 calling convention

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:70
@@ +69,3 @@
+      case ISD::BITCAST:
+      case ISD::SELECT:
+      case ISD::SELECT_CC:
----------------
I got curious why these opcodes need to be skipped and picked ISD::SELECT to look at.

Commented line 70 out does trigger error -- but further debugging indicate the bug can be in the original code. In particular, PromoteIntRes_SETCC should call ReplaceValueWith, but does not.

Fixing SETCC handling makes the test case TestMax pass.

Can you verify this is a bug triggered by f128 change?

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:158
@@ +157,3 @@
+    // When LegalInHWReg is true, we take a short-cut in SoftenFloatOperand
+    // to save repeated computations and make the code simpler there.
+    // So here we need to call ReplaceValueWith.
----------------
Where is the short-cut? Is it the new default handling in SoftenFloatOperand?  Making short cut there and have weak handshaking seems fragile. 

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:752
@@ +751,3 @@
+    // Assume all child nodes were softened if changed in place.
+    if (LegalInHWReg)
+      return false;
----------------
What are the new opcodes that reach here? Please make such opcode explict and add assertions for unexpected ones.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:791
@@ -711,2 +790,3 @@
 
-  ReplaceValueWith(SDValue(N, 0), Res);
+  if (N != Res.getNode())
+    ReplaceValueWith(SDValue(N, 0), Res);
----------------
This seems redundant -- see above early return.


http://reviews.llvm.org/D11438