[PATCH] D11438: Fix x86_64 fp128 calling convention

[Chih-hung Hsieh via llvm-commits]

David,

I noticed that our most recent emails on this thread did not show up
in http://reviews.llvm.org/D11438.
If any comment should apply to specific line of code,
please add it to http://reviews.llvm.org/D11438.

About calling GetPromotedInteger(Cond), yes, probably it will work too.
My main concern was to check for all kinds of Promote/Expand/Soften
operations like the SELECT_CC case.

Adding more assertion sounds good to me.
I think your suggested assertion #1 is as follows in LegalizeTypes.cpp,
right?

       case TargetLowering::TypeSoftenFloat:
         // If a soften float result is converted to integer,
         // assume the operands are also converted when necessary.
         // If the float result is not converted, continue to
         // scan the operands.
         Changed = SoftenFloatResult(N, i);
         if (Changed)
           goto NodeDone;
+        // If not changed, the result type should be legally in register.
+        assert(TLI.getTypeToTransformTo(*DAG.getContext(), ResultVT) ==
+               ResultVT && ResultVT.isSimple() &&
TLI.isTypeLegal(ResultVT) &&
+               "Unchanged SoftenFloatResult should be legal in register!");
         goto ScanOperands;
       case TargetLowering::TypeExpandFloat:


About your suggested 2nd check:
   ---- further check if any operands of the DAG node is not TypeLegal, if
that is true, return true to indicate Operand scanning is needed.

I think what you meant was to return false for "Changed" only when Operand
scanning is needed.  When all operand types are TypeLegal, you would like
to return true to skip ScanOperands, even though the SDNode is not changed.
This could save some computation, but it changes the meaning of "Changed"
inside DAGTypeLegalize::run(), and also repeated some function of
ScanOperands inside SoftenFloatResult.
In additional to check for
  getTypeAction(N->getOperand(i).getValueType()) ==
TargetLowering::TypeLegal
we will also need to check
  IgnoreNodeResults(N->getOperand(i).getNode())
It seems to be simpler just to reuse ScanOperands, right?

Please feel free to give me multiple comments at the same time.
Even if I cannot test them immediately, I can test multiple changes
together and run through Android code once to save some time and
get through this process sooner. Thanks.



On Tue, Nov 17, 2015 at 11:55 AM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Mon, Nov 16, 2015 at 2:57 PM, Chih-hung Hsieh <chh at google.com> wrote:
>
>> 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.
>>
>
> The DAG type legalizer follows the DU chains -- only when all the used
> operands are ready before the user is processed -- in other words, the Cond
> should have been promoted already when SELECT node is processed.
>
>
>>
>> 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?
>>
>>
> The problem with explicitly skipping those opcodes are that implicit
> assumption is made that the resulting DAG node won't be changed with f128
> and there will be remaining operands that need to be scanned.
>
> It seems to me we can do what your patch do but in a different way but
> safer way:
>   -- if the result DAG Node remains unchanged after Softening,
>    ---- assert that LegalInHWReg is true
>    ---- further check if any operands of the DAG node is not TypeLegal, if
> that is true, return true to indicate Operand scanning is needed.
>
> David
>
>
>
>
>>
>> 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/20151119/65db5188/attachment.html>