llvm dev - Mar 2013 - [LLVMdev] Bug in visitSIGN_EXTEND in DAGCombiner.cpp?

visitSIGN_EXTEND() in DAGCombiner.cpp generates an ISD::SELECT even if VT is a
vector, which causes ExpandSELECT() to assert during legalization.
I think what's required is to have visitSIGN_EXTEND generate a VSELECT if VT
is a vector…

I've tried a local change that cures this particular assert, but uncovers
another assert later, so I'm a bit uncertain if I'm heading off in the
wrong direction.
The subsequent assert comes in ExpandVSELECT, which expects (for no good reason
as far as I can tell) that the "mask" (really the 'selector')
and the operands will be the same width, rather than trying to make them the
same width (in our problem cases, the mask is an i32 produced from
getSetCCResultType() and the ops are i8 or i16 - all vectors of the same length,
of course.)

I don't really want to fix that, as the generated XOR/AND/AND/OR sequence
isn't desirable at all for our target, which has an instruction that I think
does precisely VSELECT…

Richard

Hi Richard, 

> visitSIGN_EXTEND() in DAGCombiner.cpp generates an ISD::SELECT even if VT
is a vector, which causes ExpandSELECT() to assert during legalization.
> I think what's required is to have visitSIGN_EXTEND generate a VSELECT
if VT is a vector…
ISD::SELECT should be used for cases where the selector is a scalar, even if the
operands are vector. If you found a case where SELECT is used with a vector
operand then this is a bug. We should probably add an assert in
SelectionDAG::getNode().
> 
> I've tried a local change that cures this particular assert, but
uncovers another assert later, so I'm a bit uncertain if I'm heading off
in the wrong direction.
I think that replacing SELECT with VSELECT is the right thing to do. 
> The subsequent assert comes in ExpandVSELECT, which expects (for no good
reason as far as I can tell) that the "mask" (really the
'selector') and the operands will be the same width, rather than trying
to make them the same width (in our problem cases, the mask is an i32 produced
from getSetCCResultType() and the ops are i8 or i16 - all vectors of the same
length, of course.)
If I remember correctly the trick of expanding VSELECT into a sequence of
XOR/AND/OR only works if the mask is known to be all-one or all-zero.
> 
> I don't really want to fix that, as the generated XOR/AND/AND/OR
sequence isn't desirable at all for our target, which has an instruction
that I think does precisely VSELECT…
> 
> Richard
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130308/88183a0c/attachment.html>

On Mar 8, 2013, at 2:29 PM, Nadav Rotem <nrotem at apple.com<mailto:nrotem
at apple.com>>
 wrote:

Hi Richard,


visitSIGN_EXTEND() in DAGCombiner.cpp generates an ISD::SELECT even if VT is a
vector, which causes ExpandSELECT() to assert during legalization.
I think what's required is to have visitSIGN_EXTEND generate a VSELECT if VT
is a vector…

ISD::SELECT should be used for cases where the selector is a scalar, even if the
operands are vector. If you found a case where SELECT is used with a vector
operand then this is a bug.

I did… It originates from an icmp ne <2x i8>, zero initializer followed by
a sext of the result 2x i1 to 2x i8. When we visit the SIGN_EXTEND, we generate
the ISD::SELECT even though the selector and both operands are vectors.

We should probably add an assert in SelectionDAG::getNode().

I've tried a local change that cures this particular assert, but uncovers
another assert later, so I'm a bit uncertain if I'm heading off in the
wrong direction.

I think that replacing SELECT with VSELECT is the right thing to do.

VT.isVector() ? ISD::VSELECT : ISD::SELECT in lieu of the fixed ISD::SELECT
seems to do the trick for me. Except that it sometimes hits the assert in
ExpandVSELECT...

The subsequent assert comes in ExpandVSELECT, which expects (for no good reason
as far as I can tell) that the "mask" (really the 'selector')
and the operands will be the same width, rather than trying to make them the
same width (in our problem cases, the mask is an i32 produced from
getSetCCResultType() and the ops are i8 or i16 - all vectors of the same length,
of course.)

If I remember correctly the trick of expanding VSELECT into a sequence of
XOR/AND/OR only works if the mask is known to be all-one or all-zero.

Well, that's not a problem since the mask was generated from a sign extend
from an i1… ;-) The problem comes about because the i32 mask and i8 or i16
operands are different widths and expandVSELECT is not equipped to deal with
that case.

I don't really want to fix that, as the generated XOR/AND/AND/OR sequence
isn't desirable at all for our target, which has an instruction that I think
does precisely VSELECT…

It turns out that for i8/i16 types, we'd have to promote them to i32 types
to take advantage of our instruction… so the sequence isn't as bad as I
thought at first.


Richard

_______________________________________________
LLVM Developers mailing list
LLVMdev at cs.uiuc.edu<mailto:LLVMdev at cs.uiuc.edu>        
http://llvm.cs.uiuc.edu<http://llvm.cs.uiuc.edu/>
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130311/57fe8eb0/attachment.html>