Return value from TargetLowering::LowerOperation?

Project Infrastructure LLVM Dev List Archives
1

Hi,

I'm a litle bit puzzled by the TargetLowering::LowerOperation function, and what different callers of this function assumes about the returned value.

In several places it seems like it is assumed that LowerOperation can return three kinds of values:
* Something completely new.
* SDValue()
* The same SDValue as LowerOperation was called on.

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.

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

2

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?

-Tom

3

I thought SDValue() does this. The same op is supposed to defer to the generic legalization

4

Hi,

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

5

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