On Fri, 12 Feb 2016, Jakub Jelinek wrote:
> On Fri, Feb 12, 2016 at 01:48:36PM +0100, Richard Biener wrote:
> > 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 case I'm worried about is if xop1 is a constant in narrower
> mode (let's say QImode), e.g. -15, unsignedp is 1, and xmode1 is
> wider. Then previously we'd zero extend it, thus get
> 0xf1, but with your patch we'll end up with -15 instead,
> because convert_modes will be called e.g. with (SImode, VOIDmode, xop1, 1)
> instead of (SImode, QImode, xop1, 1).
> Dunno if it is just hypothetical or real.
But we don't know which mode xop1 was expanded with here. The only
way to know would be passing down its original type (or its mode).
That's the general issue with our modeless CONST_INTs.
So yes, that case is sth to worry about (for all operations that
can arrive in expand_binop_directly which can have different mode
operands).
Also note that unsignedp doesn't apply to op1 for shifts, only to op0.
So eventually we shouldn't (ab-)use expand_binop for expanding shifts
at all...
Richard.
