[PATCH] D11438: Fix x86_64 fp128 calling convention
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 19 15:39:52 PST 2015
On Thu, Nov 19, 2015 at 3:02 PM, Chih-hung Hsieh <chh at google.com> wrote:
> 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!");
>
yes.
> 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?
>
>
A simpler solution is to return not-done/or Changed=false whenever Softened
result node R is unchanged -- basically let scanOperands to check operand
type legality. This should never happen for softfp for other floating
types. Would that work? If yes, I think it will be the
cleanest/no-intrusive change -- as there will be no special handling of
SELECT/SELECT_CC/..
Regarding other changes in SoftenFloatResult, I think the check of
LegalInHWReg should be pushed down into individual opcodes that actually
need to check it. The reason being:
1) there is no need to change interfaces of SoftenFloatRes_<OP> ..
2) it can actually save compile time. Currently it forces *all* opcode to
compute this flag while only a small subset of opcode cares about it.
3) it makes code more readable -- the change in main driver function
SoftenFloatResult is minimized, and the per-opcode logic is closer to the
actual code so it is easier to reason about. For instance, currently you
have
SoftenFloatResult (.... ) {
case ISD::FABS:
if (!LegalInHWReg)
R = SoftenFloatRes_FABS (...)
break;
which should leave unchanged, but inside
SoftenFloatRes_FABS(...) {
...
if (LegalInHWReg)
return N; // With good comments and explanation ..
// original code..
}
Does it make sense?
David
> 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/436e16f7/attachment.html>
More information about the llvm-commits
mailing list