[llvm-dev] Return value from TargetLowering::LowerOperation?

[Mikael Holmén via llvm-dev]

Hi,

In our out-of-tree backend we do

       setOperationAction(ISD::BITCAST, VT, Custom);

on all types, since we need to specially handle all casts to/from vNi16.

This has the effect that for one testcase (not dealing with vNi16 at 
all), during Legalization we come to LowerOperation on a node

  t70: v4i32 = bitcast t27

where t27 is

  t27: v2i64 = or t15, t26

On my target v4i32 is legal, v2i64 (and i64) is not.

My target's LowerOperation doesn't do anything special with the t70 
bitcast, so we just return the same node and expect the generic 
legalization code to handle it.

However, TargetLowering::LowerOperationWrapper doesn't have any check 
whether the returned SDValue is the same as the input, and thus it 
passes it on to DAGTypeLegalizer::CustomLowerNode, which tries to do 
ReplaceValueWith, and there we hit the assert
  "Potential legalization loop!"

Right now we're doing an extra check in 
TargetLowering::LowerOperationWrapper (similar to how it's done in 
SelectionDAGLegalize::LegalizeOp) to get around this issue, but we're 
unsure if this is the right thing to do or if the proper solution is 
elsewhere:

void TargetLowering::LowerOperationWrapper(SDNode *N,
                                            SmallVectorImpl<SDValue> 
&Results,
                                            SelectionDAG &DAG) const {
   if (SDValue Res = LowerOperation(SDValue(N, 0), DAG))
     // Check if LowerOperation returned the same SDValue as
     // passed into it, similar to LegalizeDAG.cpp line ~1406.
     if (Res != SDValue(N, 0))
       Results.push_back(Res);
}

We have been doing this for some time now and aren't aware of any 
problems but it would be good with some more insight around this.

Thanks,
Mikael

On 01/25/2016 08:29 AM, Mikael Holmén via llvm-dev wrote:
> Hi,
>
> On 01/22/2016 05:02 PM, Tom Stellard wrote:
>> On Fri, Jan 22, 2016 at 01:58:49PM +0100, Mikael Holmén via llvm-dev
>> wrote:
>>> Hi,
>>>
>>> I'm a litle bit puzzled by the TargetLowering::LowerOperation function,
>>> and what different callers of this function assumes about the returned
>>> value.
>>>
>> SelectionDAGLegalize::LegalizeOp() is your best reference for this.
>>
>>> In several places it seems like it is assumed that LowerOperation can
>>> return three kinds of values:
>>> * Something completely new.
>>> * SDValue()
>>
>> This tells the legalizer to treat the node as if it were marked Expand.
>> For loads and stores it tells the legalizer to treat the node as if
>> it were marked Promote.
>>
>>> * The same SDValue as LowerOperation was called on.
>>
>> This tells the legalizer that the input node is legal.
>>
>>>
>>> However in some places, e.g. in TargetLowering::LowerOperationWrapper,
>>> it seems like it is assumed only the first two of the above cases can
>>> happen because there we do:
>>>
>>>     SDValue Res = LowerOperation(SDValue(N, 0), DAG);
>>>     if (Res.getNode())
>>>       Results.push_back(Res);
>>>
>>> So, when LowerOperationWrapper is called from
>>> DAGTypeLegalizer::CustomLowerNode, and my target's LowerOperation
>>> returns the same SDValue as it was called on since we don't want to do
>>> anything special with this particular SDValue, then we get an assert
>>> "Potential legalization loop!" when DAGTypeLegalizer::CustomLowerNode
>>> tries to replace the SDValue with itself.
>>>
>>
>> I think this error can only happen during type legalization, so my guess
>> is that you are returning a node that has an illegal type.  Can you
>> provide more information about the node this is failing with?
>
> On my target v2i16, v4i16, v2i32, v4i32, v2f32, v4f32 are legal and all
> other vector types are not.
>
> Vectors of i16 are a bit special and we need to custom lower bitcasts
> to/from them. Therefore we do
>
>      setOperationAction(ISD::BITCAST, VT, Custom);
>
> on all MVT:s, and in our target's LowerOperation/LowerBitcast we
> specifically handle when the source or target type is v2i16 or v4i16,
> and for other cases we just return the input SDValue and let the
> "normal" code handle it in whatever way it see fits.
>
> In this particular case, when it crashes, we have a bitcast from v2i64
> to v4i32:
>
> t70: v4i32 = bitcast t27
>
> and t27 is
>
> t27: v2i64 = or t15, t26
>
> so it's a bitcast from v2i64 to v4i32 that we don't want to do anything
> special with so we return the input SDValue which
> TargetLowering::LowerOperationWrapper returns back to
> DAGTypeLegalizer::CustomLowerNode and then we get the assert in
> DAGTypeLegalizer::ReplaceValueWith.
>
> Thanks,
> Mikael
>
>>
>> -Tom
>>
>>> So in some cases it's ok for LowerOperation to return the same SDValue
>>> it was called on, and sometimes it's not?
>>>
>>> Can anyone shed some light on this?
>>>
>>
>>
>>
>>
>>> Thanks,
>>> Mikael
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev