On Fri, 12 Feb 2016, Richard Biener wrote:
> 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).
Oh, bootstrap & testing went fine on x86_64-unknown-linux-gnu.
If we can agree on the patch then I'll pick up the testcase from
your patch (and adjust mine).
Richard.
