[PATCH] D11438: Fix x86_64 fp128 calling convention

[Chih-Hung Hsieh via llvm-commits]

chh marked 2 inline comments as done.
chh added a comment.

Will upload diff 11 soon with suggested changes.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:70
@@ +69,3 @@
+      case ISD::BITCAST:
+      case ISD::SELECT:
+      case ISD::SELECT_CC:
----------------
davidxl wrote:
> 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?
Yes, I can reproduce that error too. I believe that it happens only with
my new x86_64 f128 configuration, but I have no way to verify for all targets
and illegal types. It's arguably a "bug" as it worked with existing ScanOperands.

However, the reason to skip some Opcode is not to workaround the problem of
missed calls to ReplaceValueWith. We want to keep some Opcode with f128 type
for instruction selection to correct calling convention or more efficient code.
When we do not change SDNode for f128 type, we need to tell LegalizeTypes to
treat it as a "legal" type and go to ScanOperands, so all other existing cases
will work with this unchanged SDNode.


================
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.
----------------
davidxl wrote:
> Where is the short-cut? Is it the new default handling in SoftenFloatOperand?  Making short cut there and have weak handshaking seems fragile. 
Yes, in SoftenFloatOperand, I do not try to repeatedly replace a SDNode
when its operand type can be kept as float and not changed.
Current LegalizeTypes and LegalizeFloatTypes depend on ScanOperands to
replace SDNode where DAGTypeLegalizer::*Result functions "missed" calls
to ReplaceValueWith. That is kind of strange and somewhat inefficient.

However, changing that architecture is too big a risk and work for me to
fix x86_64 f128 calling convention and make sure the change work for
all targets now and in the future. I don't think there are enough regression
tests for all target and types that depend on LegalizeTypes. I found all broken
cases when trying to compile libm on Android.

Hence, I use this safer and "smaller" patch to match existing architecture
and limit the impact to only SoftenFloat* and LegalInHWReg cases.
It could be a separate patch to clean up the complicated loops inside LegalizeTypes,
or LegalizeTypes could be rewritten as we know that there are non-HW-supported operations
on some types but not really "illegal types".


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


================
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);
----------------
davidxl wrote:
> This seems redundant -- see above early return.
Fixed.



http://reviews.llvm.org/D11438