[PATCH] D11438: Fix x86_64 fp128 calling convention

[Xinliang David Li via llvm-commits]

On Thu, Nov 12, 2015 at 11:06 AM, Chih-Hung Hsieh <chh at google.com> wrote:

> 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.
>
>
I tried it for arm with soft-fp and see why it works in that case (i.e.,
without calling ReplaceValueWith). What happens when SoftenFloatRes_SELECT
is called, a new SDNode is created due to f32->i32. The new node will then
be added to the worklist and re-analyzed. When it is re-analyzed, the
unpromoted integer (condition) operand of SELECT gets processed and it
works fine.

However with f128, since f128 is kept in HW register, the
SoftenFloatRes_SELECT ends up returning the original node due to CSE.  This
node does not get reanalyzed and one of its operand is an unpromoted
integer thus causes later assertion.

If the current design assumes after SoftenFloatRes_OPCODE call, all the
operands of the OPCODE node are Type Legalized, then the real bug is in
SoftenFloatRes_SELECT function. I tried a simple fix (similar to other
places) and it works just fine:

-  return DAG.getSelect(SDLoc(N),
-                       LHS.getValueType(), N->getOperand(0), LHS, RHS);
+  SDValue Cond = N->getOperand(0);
+  if (getTypeAction(Cond.getValueType()) ==
TargetLowering::TypePromoteInteger)
+    Cond = GetPromotedInteger(Cond);
+  return DAG.getSelect(SDLoc(N),
+                       LHS.getValueType(), Cond, LHS, RHS);

I think this fix should be safe for general case -- there is no need to
skip the call to SoftenFloatRes_SELECT to workaround the problem.

David





>
> ================
> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151113/3771f8b6/attachment.html>