On Fri, 12 Feb 2016, Jakub Jelinek wrote:
> On Fri, Feb 12, 2016 at 01:15:11PM +0100, Bernd Schmidt wrote:
> > >Ah, indeed looks like a dup. Let's go with your version which had
> > >feedback from Bernd already. Might want to add my testcase (w/o the
> > >runtime outcome test).
> >
> > Actually, Richard I was just looking at Jakub's second patch and I think
> > yours is better - on the first round of review I didn't notice that the
> > convert_modes code is there and is documented to deal with the CONST_INT
> > problem. If it completed testing I think you should commit it.
>
> That patch doesn't look right to me. The code is there not just for shifts,
> but also for non-shifts, and for non-shifts, we know the arguments are in
> mode, we also know unsignedp, so if needed convert_modes can perform
> zero or sign extension. IMHO it is just shifts/rotates that are
> problematical, because what the second operand mode is and whether it is
> unsigned
> or signed is just less well defined, and then various backends have various
> requirements on it. Also, on some target for shift/rotate xmode1 might be
> already equal to mode, and in that case convert_modes would not be called,
> but still the CONST_INT might be originally from yet another mode and we'd
> still ICE.
But my patch should deal with all this.
- mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
- if (xmode1 != VOIDmode && xmode1 != mode1)
+ mode1 = GET_MODE (xop1);
+ if (xmode1 != mode1)
{
xop1 = convert_modes (xmode1, mode1, xop1, unsignedp);
mode1 = xmode1;
so if xop1 is not VOIDmode and already of xmode1 we won't do anything.
But if it is VOIDmode (and xmode1 is not VOIDmode) we'll always
do convert_modes.
The only thing that can (hopefully not) happen is that xmode1
is VOIDmode but mode1 is not (maybe removing the xmode1 != VOIDmode
check is a bit too optimistic here). Not sure if there can be
valid patterns requesting a VOIDmode op ... (and not only accept
CONST_INTs).
Richard.
