[PATCH] D11438: Fix x86_64 fp128 calling convention

[Chih-hung Hsieh via llvm-commits]

David,

Although I said that ScanOperands after the legalizing-result-type loop is
strange and might be inefficient, I feel that it is safer and simpler to
reuse existing flow than embedding some legalization of operands into the
legalize-result-type functions.

I don't think the current design assumes all operands to be "legal" after
the legalizing-result-type loop. The operands are required to be legal only
when the node is "changed". When IgnoredNodeRsults(N) is true, or when the
type action is TypeLegal, ScanOperands is executed.

Your fix is similar to embedding PromoteIntOp_SELECT into
SoftenFloatRes_SELECT.
In that case, I think GetPromotedInteger(Cond) will work only if that Cond
value was promoted before. What needed is probably a call to
PromoteIntOp_SELECT inside SoftenFloatRes_SELECT.

But, how do we know that SoftenFloatRes_SELECT's "illegal" operand could
only be integers to be promoted? Could it require expand or soften? I don't
see other Op_SELECT now, but if a new illegal SELECT operand type is added
later, we will have to add that to multiple places. So, embedding *Op_XYZ
into *Res_XYZ now will require a check of all possible type actions of the
operand.

SELECT_CC is more complicated. I see 5 kinds of Op_SELECT_CC functions now:
  PromoteIntOp_SELECT_CC
  ExpandIntOp_SELECT_CC
  SoftenFloatOp_SELECT_CC
  ExpandFloatOp_SELECT_CC
  PromoteFloatOp_SELECT_CC

So it seems safer and less repeated code now to reuse the ScanOperands loop.
We only need to return false for the "changed" flag from SoftenFloatResult
or any legalizing-result-type function.
Does that make sense to you?


On Fri, Nov 13, 2015 at 5:53 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> 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/20151116/54124059/attachment.html>