WidenVectorOperand() bug?

[Friedman, Eli via llvm-commits]

On 1/18/2017 6:13 AM, Jonas Paulsson via llvm-commits wrote:
> Hi,
>
> I wanted to add a custom handling of ISD::SIGN_EXTEND in 
> ReplaceNodeResults(), so that
> a sext of v2i16->v2i32 would be done the same way as a v4i16 -> v4i32. 
> First, I added
>
> setOperationAction(ISD::SIGN_EXTEND, MVT::v2i32, Custom);
>
> This all worked out well after a while...
>
> To my surprise though, another test case then failed, with a sext 
> v2i32 -> v2i64. The fail
> is caused by the sext end up in LowerOperation(), where it isn't 
> handled and so an assert
> triggers.
>
> This is quite weird, I think - to have the SIGN_EXTEND of *another 
> type* than given with setOperationAction() all of a sudden get custom 
> lowered!
>
> What happens is that in WidenVectorOperand(), this code:
>
> // See if the target wants to custom widen this node.
>   if (CustomLowerNode(N, N->getOperand(OpNo).getValueType(), false))
>     return false;
>
> checks for custom lowering a sext of v2i32, while N is actually a sext 
> v2i64!
>
> I can't imagine this to be right so I am guessing that usually the 
> operands and results have the same value type, so this usually doesn't 
> matter, and so this is in fact a bug?

This is a little confusing, but I wouldn't call it a bug.  We 
consistently call CustomLowerNode with the illegal type, whether it's 
the result or an operand, and existing targets depend on that.

> On the other hand, with this patch plenty of regression tests in the 
> X86 backend fail, so please help me out. (If this is the way the code 
> must behave, I suppose then LowerOperation() must then return 'Op' 
> unchanged when the sext we don't want to custom handle appears...?)

If you want the legalizer to use its default expansion for a node, you 
should just return SDNode().

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project