[PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Richard Biener]

I am testing the following patch which fixes PR69771 where the code
doesn't match the comment before it.  We get to expand a QImode << QImode
shift but the shift amount was of type int and thus it was expanded
as SImode constant.  Then

  /* In case the insn wants input operands in modes different from
     those of the actual operands, convert the operands.  It would
     seem that we don't need to convert CONST_INTs, but we do, so
     that they're properly zero-extended, sign-extended or truncated
     for their mode.  */

has to apply as we need to re-extend the VOIDmode CONST_INT for
QImode.  But then mode1 is computed as 'mode' (QImode) which happens
to match what is expected even though the constant isn't valid.

The fix is IMHO to always call convert_modes for VOIDmode ops
(if the target doesn't expect VOIDmode itself).

Bootstrap and regtest running on x86_64-unknown-linux-gnu, ok for trunk?

Thanks,
Richard.

2016-02-12  Richard Biener  <rguenther@suse.de>

	PR rtl-optimization/69771
	* optabs.c (expand_binop_directly): Properly zero-/sign-extend
	VOIDmode operands.

	* gcc.dg/torture/pr69771.c: New testcase.

Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c	(revision 233369)
+++ gcc/optabs.c	(working copy)
@@ -1013,15 +1013,15 @@ expand_binop_directly (machine_mode mode
      that they're properly zero-extended, sign-extended or truncated
      for their mode.  */
 
-  mode0 = GET_MODE (xop0) != VOIDmode ? GET_MODE (xop0) : mode;
-  if (xmode0 != VOIDmode && xmode0 != mode0)
+  mode0 = GET_MODE (xop0);
+  if (xmode0 != mode0)
     {
       xop0 = convert_modes (xmode0, mode0, xop0, unsignedp);
       mode0 = xmode0;
     }
 
-  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;
Index: gcc/testsuite/gcc.dg/torture/pr69771.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr69771.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr69771.c	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+
+unsigned char a = 5, c;
+unsigned short b = 0;
+unsigned d = 0x76543210;
+void __attribute__((noinline))
+fn1() { c = d >> ~(a || ~b); } /* { dg-warning "shift count is negative" } */
+
+int main()
+{
+  fn1();
+  if (c != 1)
+    __builtin_abort ();
+  return 0;
+}

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Jakub Jelinek]

On Fri, Feb 12, 2016 at 11:23:26AM +0100, Richard Biener wrote:
> 
> I am testing the following patch which fixes PR69771 where the code
> doesn't match the comment before it.  We get to expand a QImode << QImode
> shift but the shift amount was of type int and thus it was expanded
> as SImode constant.  Then
> 
>   /* In case the insn wants input operands in modes different from
>      those of the actual operands, convert the operands.  It would
>      seem that we don't need to convert CONST_INTs, but we do, so
>      that they're properly zero-extended, sign-extended or truncated
>      for their mode.  */
> 
> has to apply as we need to re-extend the VOIDmode CONST_INT for
> QImode.  But then mode1 is computed as 'mode' (QImode) which happens
> to match what is expected even though the constant isn't valid.
> 
> The fix is IMHO to always call convert_modes for VOIDmode ops
> (if the target doesn't expect VOIDmode itself).
> 
> Bootstrap and regtest running on x86_64-unknown-linux-gnu, ok for trunk?

This looks like the PR69764 fix I've sent last night (and updated patch
this morning).
BTW, I wouldn't use a runtime test with clearly undefined behavior,
especially not if it tests what the outcome of that UB is.

> 2016-02-12  Richard Biener  <rguenther@suse.de>
> 
> 	PR rtl-optimization/69771
> 	* optabs.c (expand_binop_directly): Properly zero-/sign-extend
> 	VOIDmode operands.
> 
> 	* gcc.dg/torture/pr69771.c: New testcase.

	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Richard Biener]

On Fri, 12 Feb 2016, Jakub Jelinek wrote:

> On Fri, Feb 12, 2016 at 11:23:26AM +0100, Richard Biener wrote:
> > 
> > I am testing the following patch which fixes PR69771 where the code
> > doesn't match the comment before it.  We get to expand a QImode << QImode
> > shift but the shift amount was of type int and thus it was expanded
> > as SImode constant.  Then
> > 
> >   /* In case the insn wants input operands in modes different from
> >      those of the actual operands, convert the operands.  It would
> >      seem that we don't need to convert CONST_INTs, but we do, so
> >      that they're properly zero-extended, sign-extended or truncated
> >      for their mode.  */
> > 
> > has to apply as we need to re-extend the VOIDmode CONST_INT for
> > QImode.  But then mode1 is computed as 'mode' (QImode) which happens
> > to match what is expected even though the constant isn't valid.
> > 
> > The fix is IMHO to always call convert_modes for VOIDmode ops
> > (if the target doesn't expect VOIDmode itself).
> > 
> > Bootstrap and regtest running on x86_64-unknown-linux-gnu, ok for trunk?
> 
> This looks like the PR69764 fix I've sent last night (and updated patch
> this morning).
> BTW, I wouldn't use a runtime test with clearly undefined behavior,
> especially not if it tests what the outcome of that UB is.

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).

Richard.

> > 2016-02-12  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR rtl-optimization/69771
> > 	* optabs.c (expand_binop_directly): Properly zero-/sign-extend
> > 	VOIDmode operands.
> > 
> > 	* gcc.dg/torture/pr69771.c: New testcase.
> 
> 	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Bernd Schmidt]

On 02/12/2016 11:46 AM, Richard Biener wrote:
> On Fri, 12 Feb 2016, Jakub Jelinek wrote:
>
>> On Fri, Feb 12, 2016 at 11:23:26AM +0100, Richard Biener wrote:
>>>
>>> I am testing the following patch which fixes PR69771 where the code
>>> doesn't match the comment before it.  We get to expand a QImode << QImode
>>> shift but the shift amount was of type int and thus it was expanded
>>> as SImode constant.  Then
>>>
>>>    /* In case the insn wants input operands in modes different from
>>>       those of the actual operands, convert the operands.  It would
>>>       seem that we don't need to convert CONST_INTs, but we do, so
>>>       that they're properly zero-extended, sign-extended or truncated
>>>       for their mode.  */
>>>
>>> has to apply as we need to re-extend the VOIDmode CONST_INT for
>>> QImode.  But then mode1 is computed as 'mode' (QImode) which happens
>>> to match what is expected even though the constant isn't valid.
>>>
>>> The fix is IMHO to always call convert_modes for VOIDmode ops
>>> (if the target doesn't expect VOIDmode itself).
>>>
>>> Bootstrap and regtest running on x86_64-unknown-linux-gnu, ok for trunk?
>>
>> This looks like the PR69764 fix I've sent last night (and updated patch
>> this morning).
>> BTW, I wouldn't use a runtime test with clearly undefined behavior,
>> especially not if it tests what the outcome of that UB is.
>
> 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.


Bernd

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Jakub Jelinek]

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.

	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Richard Biener]

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.

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Richard Biener]

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.

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Jakub Jelinek]

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.

	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Jakub Jelinek]

On Fri, Feb 12, 2016 at 01:50:08PM +0100, Richard Biener wrote:
> > -  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).

Another possibility, only do the convert_modes from VOIDmode for
shift_optab_p's xop1, and keep doing what we've done before otherwise.

2016-02-12  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/69764
	PR rtl-optimization/69771
	* optabs.c (expand_binop_directly): For shift_optab_p, force
	convert_modes with VOIDmode if xop1 has VOIDmode.

	* c-c++-common/pr69764.c: New test.
	* gcc.dg/torture/pr69771.c: New testcase.

--- gcc/optabs.c.jj	2016-02-12 00:50:30.410240366 +0100
+++ gcc/optabs.c	2016-02-12 10:33:32.743492532 +0100
@@ -988,7 +988,7 @@ expand_binop_directly (machine_mode mode
 						      from_mode, 1);
   machine_mode xmode0 = insn_data[(int) icode].operand[1].mode;
   machine_mode xmode1 = insn_data[(int) icode].operand[2].mode;
-  machine_mode mode0, mode1, tmp_mode;
+  machine_mode mode0, mode1 = mode, tmp_mode;
   struct expand_operand ops[3];
   bool commutative_p;
   rtx_insn *pat;
@@ -1006,6 +1006,8 @@ expand_binop_directly (machine_mode mode
   xop0 = avoid_expensive_constant (xmode0, binoptab, 0, xop0, unsignedp);
   if (!shift_optab_p (binoptab))
     xop1 = avoid_expensive_constant (xmode1, binoptab, 1, xop1, unsignedp);
+  else
+    mode1 = VOIDmode;
 
   /* In case the insn wants input operands in modes different from
      those of the actual operands, convert the operands.  It would
@@ -1020,7 +1020,7 @@ expand_binop_directly (machine_mode mode
       mode0 = xmode0;
     }
 
-  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
+  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode1;
   if (xmode1 != VOIDmode && xmode1 != mode1)
     {
       xop1 = convert_modes (xmode1, mode1, xop1, unsignedp);
--- gcc/testsuite/c-c++-common/pr69764.c.jj	2016-02-12 10:27:34.522587995 +0100
+++ gcc/testsuite/c-c++-common/pr69764.c	2016-02-12 10:27:34.522587995 +0100
@@ -0,0 +1,38 @@
+/* PR rtl-optimization/69764 */
+/* { dg-do compile { target int32plus } } */
+
+unsigned char
+fn1 (unsigned char a)
+{
+  return a >> ~6;	/* { dg-warning "right shift count is negative" } */
+}
+
+unsigned short
+fn2 (unsigned short a)
+{
+  return a >> ~6;	/* { dg-warning "right shift count is negative" } */
+}
+
+unsigned int
+fn3 (unsigned int a)
+{
+  return a >> ~6;	/* { dg-warning "right shift count is negative" } */
+}
+
+unsigned char
+fn4 (unsigned char a)
+{
+  return a >> 0xff03;	/* { dg-warning "right shift count >= width of type" } */
+}
+
+unsigned short
+fn5 (unsigned short a)
+{
+  return a >> 0xff03;	/* { dg-warning "right shift count >= width of type" } */
+}
+
+unsigned int
+fn6 (unsigned int a)
+{
+  return a >> 0xff03;	/* { dg-warning "right shift count >= width of type" } */
+}
--- gcc/testsuite/gcc.dg/torture/pr69771.c.jj	2016-02-12 10:27:34.522587995 +0100
+++ gcc/testsuite/gcc.dg/torture/pr69771.c	2016-02-12 10:27:34.522587995 +0100
@@ -0,0 +1,12 @@
+/* PR rtl-optimization/69771 */
+/* { dg-do compile } */
+
+unsigned char a = 5, c;
+unsigned short b = 0;
+unsigned d = 0x76543210;
+
+void
+foo (void)
+{
+  c = d >> ~(a || ~b);	/* { dg-warning "shift count is negative" } */
+}

	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Richard Biener]

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.

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Richard Biener]

On Fri, 12 Feb 2016, Jakub Jelinek wrote:

> On Fri, Feb 12, 2016 at 01:50:08PM +0100, Richard Biener wrote:
> > > -  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).
> 
> Another possibility, only do the convert_modes from VOIDmode for
> shift_optab_p's xop1, and keep doing what we've done before otherwise.

That looks like a very targeted and safe fix indeed.

Richard.

> 2016-02-12  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/69764
> 	PR rtl-optimization/69771
> 	* optabs.c (expand_binop_directly): For shift_optab_p, force
> 	convert_modes with VOIDmode if xop1 has VOIDmode.
> 
> 	* c-c++-common/pr69764.c: New test.
> 	* gcc.dg/torture/pr69771.c: New testcase.
> 
> --- gcc/optabs.c.jj	2016-02-12 00:50:30.410240366 +0100
> +++ gcc/optabs.c	2016-02-12 10:33:32.743492532 +0100
> @@ -988,7 +988,7 @@ expand_binop_directly (machine_mode mode
>  						      from_mode, 1);
>    machine_mode xmode0 = insn_data[(int) icode].operand[1].mode;
>    machine_mode xmode1 = insn_data[(int) icode].operand[2].mode;
> -  machine_mode mode0, mode1, tmp_mode;
> +  machine_mode mode0, mode1 = mode, tmp_mode;
>    struct expand_operand ops[3];
>    bool commutative_p;
>    rtx_insn *pat;
> @@ -1006,6 +1006,8 @@ expand_binop_directly (machine_mode mode
>    xop0 = avoid_expensive_constant (xmode0, binoptab, 0, xop0, unsignedp);
>    if (!shift_optab_p (binoptab))
>      xop1 = avoid_expensive_constant (xmode1, binoptab, 1, xop1, unsignedp);
> +  else
> +    mode1 = VOIDmode;
>  
>    /* In case the insn wants input operands in modes different from
>       those of the actual operands, convert the operands.  It would
> @@ -1020,7 +1020,7 @@ expand_binop_directly (machine_mode mode
>        mode0 = xmode0;
>      }
>  
> -  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
> +  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode1;
>    if (xmode1 != VOIDmode && xmode1 != mode1)
>      {
>        xop1 = convert_modes (xmode1, mode1, xop1, unsignedp);
> --- gcc/testsuite/c-c++-common/pr69764.c.jj	2016-02-12 10:27:34.522587995 +0100
> +++ gcc/testsuite/c-c++-common/pr69764.c	2016-02-12 10:27:34.522587995 +0100
> @@ -0,0 +1,38 @@
> +/* PR rtl-optimization/69764 */
> +/* { dg-do compile { target int32plus } } */
> +
> +unsigned char
> +fn1 (unsigned char a)
> +{
> +  return a >> ~6;	/* { dg-warning "right shift count is negative" } */
> +}
> +
> +unsigned short
> +fn2 (unsigned short a)
> +{
> +  return a >> ~6;	/* { dg-warning "right shift count is negative" } */
> +}
> +
> +unsigned int
> +fn3 (unsigned int a)
> +{
> +  return a >> ~6;	/* { dg-warning "right shift count is negative" } */
> +}
> +
> +unsigned char
> +fn4 (unsigned char a)
> +{
> +  return a >> 0xff03;	/* { dg-warning "right shift count >= width of type" } */
> +}
> +
> +unsigned short
> +fn5 (unsigned short a)
> +{
> +  return a >> 0xff03;	/* { dg-warning "right shift count >= width of type" } */
> +}
> +
> +unsigned int
> +fn6 (unsigned int a)
> +{
> +  return a >> 0xff03;	/* { dg-warning "right shift count >= width of type" } */
> +}
> --- gcc/testsuite/gcc.dg/torture/pr69771.c.jj	2016-02-12 10:27:34.522587995 +0100
> +++ gcc/testsuite/gcc.dg/torture/pr69771.c	2016-02-12 10:27:34.522587995 +0100
> @@ -0,0 +1,12 @@
> +/* PR rtl-optimization/69771 */
> +/* { dg-do compile } */
> +
> +unsigned char a = 5, c;
> +unsigned short b = 0;
> +unsigned d = 0x76543210;
> +
> +void
> +foo (void)
> +{
> +  c = d >> ~(a || ~b);	/* { dg-warning "shift count is negative" } */
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Jakub Jelinek]

On Fri, Feb 12, 2016 at 02:45:40PM +0100, Richard Biener wrote:
> > 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).

Well, most binary ops have the requirement that both modes are the same,
which is why most of the expansion APIs for them pass around just a single
mode, not two modes (or 3, if even the result could have different mode).
And in that case we better have expanded the CONST_INTs with the right mode
already.  Just shifts are special.

> 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...

Perhaps; but please look at the latest patch I've posted, which IMHO does
what your patch does for shift/rorate xop1 only, and keeps doing what it
used to do otherwise.

	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Bernd Schmidt]

On 02/12/2016 02:47 PM, Richard Biener wrote:
>> Another possibility, only do the convert_modes from VOIDmode for
>> shift_optab_p's xop1, and keep doing what we've done before otherwise.
>
> That looks like a very targeted and safe fix indeed.

You two can obviously go ahead and sort this out, I'll just make one 
more comment. What attracted me to Richard's patch was its clarity and 
lack of potential to confuse readers.

>> -  machine_mode mode0, mode1, tmp_mode;
>> +  machine_mode mode0, mode1 = mode, tmp_mode;

>>     if (!shift_optab_p (binoptab))
>>       xop1 = avoid_expensive_constant (xmode1, binoptab, 1, xop1, unsignedp);
>> +  else
>> +    mode1 = VOIDmode;

>> -  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
>> +  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode1;
>>     if (xmode1 != VOIDmode && xmode1 != mode1)
>>       {

Here, things aren't so clear, and the fact that the mode1 calculation 
now differs from the mode0 one may be overlooked by someone in the future.

Rather than use codes like "mode variable is VOIDmode", I'd prefer to 
use booleans with descriptive names, like "op1_may_need_conversion".


Bernd

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Jakub Jelinek]

On Fri, Feb 12, 2016 at 03:20:07PM +0100, Bernd Schmidt wrote:
> >>-  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
> >>+  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode1;
> >>    if (xmode1 != VOIDmode && xmode1 != mode1)
> >>      {
> 
> Here, things aren't so clear, and the fact that the mode1 calculation now
> differs from the mode0 one may be overlooked by someone in the future.
> 
> Rather than use codes like "mode variable is VOIDmode", I'd prefer to use
> booleans with descriptive names, like "op1_may_need_conversion".

So do you prefer e.g. following?  Bootstrapped/regtested on x86_64-linux and
i686-linux.

2016-02-12  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/69764
	PR rtl-optimization/69771
	* optabs.c (expand_binop_directly): For shift_optab_p, force
	convert_modes with VOIDmode if xop1 has VOIDmode.

	* c-c++-common/pr69764.c: New test.
	* gcc.dg/torture/pr69771.c: New testcase.

--- gcc/optabs.c.jj	2016-02-12 00:50:30.410240366 +0100
+++ gcc/optabs.c	2016-02-12 10:33:32.743492532 +0100
@@ -993,6 +993,7 @@ expand_binop_directly (machine_mode mode
   bool commutative_p;
   rtx_insn *pat;
   rtx xop0 = op0, xop1 = op1;
+  bool canonicalize_op1 = false;
 
   /* If it is a commutative operator and the modes would match
      if we would swap the operands, we can save the conversions.  */
@@ -1006,6 +1007,11 @@ expand_binop_directly (machine_mode mode
   xop0 = avoid_expensive_constant (xmode0, binoptab, 0, xop0, unsignedp);
   if (!shift_optab_p (binoptab))
     xop1 = avoid_expensive_constant (xmode1, binoptab, 1, xop1, unsignedp);
+  else
+    /* Shifts and rotates often use a different mode for op1 from op0;
+       for VOIDmode constants we don't know the mode, so force it
+       to be canonicalized using convert_modes.  */
+    canonicalize_op1 = true;
 
   /* In case the insn wants input operands in modes different from
      those of the actual operands, convert the operands.  It would
@@ -1020,7 +1026,8 @@ expand_binop_directly (machine_mode mode
       mode0 = xmode0;
     }
 
-  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
+  mode1 = (GET_MODE (xop1) != VOIDmode || canonicalize_op1)
+	  ? GET_MODE (xop1) : mode;
   if (xmode1 != VOIDmode && xmode1 != mode1)
     {
       xop1 = convert_modes (xmode1, mode1, xop1, unsignedp);
--- gcc/testsuite/c-c++-common/pr69764.c.jj	2016-02-12 10:27:34.522587995 +0100
+++ gcc/testsuite/c-c++-common/pr69764.c	2016-02-12 10:27:34.522587995 +0100
@@ -0,0 +1,38 @@
+/* PR rtl-optimization/69764 */
+/* { dg-do compile { target int32plus } } */
+
+unsigned char
+fn1 (unsigned char a)
+{
+  return a >> ~6;	/* { dg-warning "right shift count is negative" } */
+}
+
+unsigned short
+fn2 (unsigned short a)
+{
+  return a >> ~6;	/* { dg-warning "right shift count is negative" } */
+}
+
+unsigned int
+fn3 (unsigned int a)
+{
+  return a >> ~6;	/* { dg-warning "right shift count is negative" } */
+}
+
+unsigned char
+fn4 (unsigned char a)
+{
+  return a >> 0xff03;	/* { dg-warning "right shift count >= width of type" } */
+}
+
+unsigned short
+fn5 (unsigned short a)
+{
+  return a >> 0xff03;	/* { dg-warning "right shift count >= width of type" } */
+}
+
+unsigned int
+fn6 (unsigned int a)
+{
+  return a >> 0xff03;	/* { dg-warning "right shift count >= width of type" } */
+}
--- gcc/testsuite/gcc.dg/torture/pr69771.c.jj	2016-02-12 10:27:34.522587995 +0100
+++ gcc/testsuite/gcc.dg/torture/pr69771.c	2016-02-12 10:27:34.522587995 +0100
@@ -0,0 +1,12 @@
+/* PR rtl-optimization/69771 */
+/* { dg-do compile } */
+
+unsigned char a = 5, c;
+unsigned short b = 0;
+unsigned d = 0x76543210;
+
+void
+foo (void)
+{
+  c = d >> ~(a || ~b);	/* { dg-warning "shift count is negative" } */
+}


	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Bernd Schmidt]

> So do you prefer e.g. following?  Bootstrapped/regtested on x86_64-linux and
> i686-linux.
>
> -  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
> +  mode1 = (GET_MODE (xop1) != VOIDmode || canonicalize_op1)
> +	  ? GET_MODE (xop1) : mode;

Placement of parentheses is wrong for formatting, but otherwise I think 
this patch is good.


Bernd

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[James Greenhalgh]

On Fri, Feb 12, 2016 at 05:34:21PM +0100, Jakub Jelinek wrote:
> On Fri, Feb 12, 2016 at 03:20:07PM +0100, Bernd Schmidt wrote:
> > >>-  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
> > >>+  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode1;
> > >>    if (xmode1 != VOIDmode && xmode1 != mode1)
> > >>      {
> > 
> > Here, things aren't so clear, and the fact that the mode1 calculation now
> > differs from the mode0 one may be overlooked by someone in the future.
> > 
> > Rather than use codes like "mode variable is VOIDmode", I'd prefer to use
> > booleans with descriptive names, like "op1_may_need_conversion".
> 
> So do you prefer e.g. following?  Bootstrapped/regtested on x86_64-linux and
> i686-linux.
> 
> 2016-02-12  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/69764
> 	PR rtl-optimization/69771
> 	* optabs.c (expand_binop_directly): For shift_optab_p, force
> 	convert_modes with VOIDmode if xop1 has VOIDmode.
> 
> 	* c-c++-common/pr69764.c: New test.
> 	* gcc.dg/torture/pr69771.c: New testcase.
> 

These two new tests are failing for me on AArch64 as so:

.../gcc/testsuite/c-c++-common/pr69764.c:7:12: internal compiler error: in decompose, at rtl.h:2107
0x7d30be wi::int_traits<std::pair<rtx_def*, machine_mode> >::decompose(long*, unsigned int, std::pair<rtx_def*, machine_mode> const&)
        .../gcc/rtl.h:2105
0x7d30be wide_int_ref_storage<false>::wide_int_ref_storage<std::pair<rtx_def*, machine_mode> >(std::pair<rtx_def*, machine_mode> const&)
        .../gcc/wide-int.h:936
0x7d30be generic_wide_int<wide_int_ref_storage<false> >::generic_wide_int<std::pair<rtx_def*, machine_mode> >(std::pair<rtx_def*, machine_mode> const&)
        .../gcc/wide-int.h:714
0x7d30be convert_modes(machine_mode, machine_mode, rtx_def*, int)
        .../gcc/expr.c:697
0x9ec7c6 widen_operand
        .../gcc/optabs.c:208
0x9f1e79 expand_binop(machine_mode, optab_tag, rtx_def*, rtx_def*, rtx_def*, int, optab_methods)
        .../gcc/optabs.c:1280
0x7b7a95 expand_shift_1
        .../gcc/expmed.c:2458
0x7bca49 expand_variable_shift(tree_code, machine_mode, rtx_def*, tree_node*, rtx_def*, int)
        .../gcc/expmed.c:2517
0x7e1d43 expand_expr_real_2(separate_ops*, rtx_def*, machine_mode, expand_modifier)
        .../gcc/expr.c:9029
0x6d9ed9 expand_gimple_stmt_1
        .../gcc/cfgexpand.c:3642
0x6d9ed9 expand_gimple_stmt
        .../gcc/cfgexpand.c:3702
0x6dc369 expand_gimple_basic_block
        .../gcc/cfgexpand.c:5708
0x6dfcdc execute
        .../gcc/cfgexpand.c:6323
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <http://gcc.gnu.org/bugs.html> for instructions.

I can't dig deeper today, but I will look closer on Monday if the fix is
not obvious.

Thanks,
James

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Oleg Endo]

On Sat, 2016-02-13 at 07:50 +0000, James Greenhalgh wrote:

> > So do you prefer e.g. following?  Bootstrapped/regtested on x86_64
> > -linux and
> > i686-linux.
> > 
> > 2016-02-12  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR rtl-optimization/69764
> > 	PR rtl-optimization/69771
> > 	* optabs.c (expand_binop_directly): For shift_optab_p, force
> > 	convert_modes with VOIDmode if xop1 has VOIDmode.
> > 
> > 	* c-c++-common/pr69764.c: New test.
> > 	* gcc.dg/torture/pr69771.c: New testcase.
> > 
> 
> These two new tests are failing for me on AArch64 as so:
> 

I see the same failures on SH, too.

Cheers,
Oleg

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Jakub Jelinek]

On Sat, Feb 13, 2016 at 07:50:25AM +0000, James Greenhalgh wrote:
> On Fri, Feb 12, 2016 at 05:34:21PM +0100, Jakub Jelinek wrote:
> > On Fri, Feb 12, 2016 at 03:20:07PM +0100, Bernd Schmidt wrote:
> > > >>-  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
> > > >>+  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode1;
> > > >>    if (xmode1 != VOIDmode && xmode1 != mode1)
> > > >>      {
> > > 
> > > Here, things aren't so clear, and the fact that the mode1 calculation now
> > > differs from the mode0 one may be overlooked by someone in the future.
> > > 
> > > Rather than use codes like "mode variable is VOIDmode", I'd prefer to use
> > > booleans with descriptive names, like "op1_may_need_conversion".
> > 
> > So do you prefer e.g. following?  Bootstrapped/regtested on x86_64-linux and
> > i686-linux.
> > 
> > 2016-02-12  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR rtl-optimization/69764
> > 	PR rtl-optimization/69771
> > 	* optabs.c (expand_binop_directly): For shift_optab_p, force
> > 	convert_modes with VOIDmode if xop1 has VOIDmode.
> > 
> > 	* c-c++-common/pr69764.c: New test.
> > 	* gcc.dg/torture/pr69771.c: New testcase.
> > 
> 
> These two new tests are failing for me on AArch64 as so:

As I said earlier, I wanted to fix it in expand_binop_directly because the
higher levels still GEN_INT the various shift counters and then call
expand_binop_directly.  But, as can be seen on aarch64/arm/m68k, there are
cases that need op1 to be valid for mode already in expand_binop, so in
addition to the already committed fix I think we need to handle it
at the expand_binop level too.
As we don't have a single spot with convert_modes like
expand_binop_directly, I think the best is to do there a change
for the uncommon and invalid cases only, like (seems to fix the ICE
both on aarch64 and m68k):

2016-02-15  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/69764
	PR rtl-optimization/69771
	* optabs.c (expand_binop): Ensure for shift optabs invalid CONST_INT
	op1 is valid for mode.

--- gcc/optabs.c.jj	2016-02-12 17:49:25.000000000 +0100
+++ gcc/optabs.c	2016-02-15 16:15:53.983673792 +0100
@@ -1125,6 +1125,12 @@ expand_binop (machine_mode mode, optab b
       op1 = negate_rtx (mode, op1);
       binoptab = add_optab;
     }
+  /* For shifts, constant invalid op1 might be expanded from different
+     mode than MODE.  */
+  else if (CONST_INT_P (op1)
+	   && shift_optab_p (binoptab)
+	   && UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode)))
+    op1 = gen_int_mode (INTVAL (op1), mode);
 
   /* Record where to delete back to if we backtrack.  */
   last = get_last_insn ();


	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Richard Biener]

On February 15, 2016 4:34:38 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Sat, Feb 13, 2016 at 07:50:25AM +0000, James Greenhalgh wrote:
>> On Fri, Feb 12, 2016 at 05:34:21PM +0100, Jakub Jelinek wrote:
>> > On Fri, Feb 12, 2016 at 03:20:07PM +0100, Bernd Schmidt wrote:
>> > > >>-  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) :
>mode;
>> > > >>+  mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) :
>mode1;
>> > > >>    if (xmode1 != VOIDmode && xmode1 != mode1)
>> > > >>      {
>> > > 
>> > > Here, things aren't so clear, and the fact that the mode1
>calculation now
>> > > differs from the mode0 one may be overlooked by someone in the
>future.
>> > > 
>> > > Rather than use codes like "mode variable is VOIDmode", I'd
>prefer to use
>> > > booleans with descriptive names, like "op1_may_need_conversion".
>> > 
>> > So do you prefer e.g. following?  Bootstrapped/regtested on
>x86_64-linux and
>> > i686-linux.
>> > 
>> > 2016-02-12  Jakub Jelinek  <jakub@redhat.com>
>> > 
>> > 	PR rtl-optimization/69764
>> > 	PR rtl-optimization/69771
>> > 	* optabs.c (expand_binop_directly): For shift_optab_p, force
>> > 	convert_modes with VOIDmode if xop1 has VOIDmode.
>> > 
>> > 	* c-c++-common/pr69764.c: New test.
>> > 	* gcc.dg/torture/pr69771.c: New testcase.
>> > 
>> 
>> These two new tests are failing for me on AArch64 as so:
>
>As I said earlier, I wanted to fix it in expand_binop_directly because
>the
>higher levels still GEN_INT the various shift counters and then call
>expand_binop_directly.  But, as can be seen on aarch64/arm/m68k, there
>are
>cases that need op1 to be valid for mode already in expand_binop, so in
>addition to the already committed fix I think we need to handle it
>at the expand_binop level too.
>As we don't have a single spot with convert_modes like
>expand_binop_directly, I think the best is to do there a change
>for the uncommon and invalid cases only, like (seems to fix the ICE
>both on aarch64 and m68k):

We could also force_reg those at expansion or apply SHIFT_COUNT_TRUNCATED to those invalid constants there.

Richard.

>2016-02-15  Jakub Jelinek  <jakub@redhat.com>
>
>	PR rtl-optimization/69764
>	PR rtl-optimization/69771
>	* optabs.c (expand_binop): Ensure for shift optabs invalid CONST_INT
>	op1 is valid for mode.
>
>--- gcc/optabs.c.jj	2016-02-12 17:49:25.000000000 +0100
>+++ gcc/optabs.c	2016-02-15 16:15:53.983673792 +0100
>@@ -1125,6 +1125,12 @@ expand_binop (machine_mode mode, optab b
>       op1 = negate_rtx (mode, op1);
>       binoptab = add_optab;
>     }
>+  /* For shifts, constant invalid op1 might be expanded from different
>+     mode than MODE.  */
>+  else if (CONST_INT_P (op1)
>+	   && shift_optab_p (binoptab)
>+	   && UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode)))
>+    op1 = gen_int_mode (INTVAL (op1), mode);
> 
>   /* Record where to delete back to if we backtrack.  */
>   last = get_last_insn ();
>
>
>	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Jakub Jelinek]

On Mon, Feb 15, 2016 at 06:58:45PM +0100, Richard Biener wrote:
> We could also force_reg those at expansion or apply SHIFT_COUNT_TRUNCATED to those invalid constants there.

Sure, but for force_reg we'd still need the gen_int_mode anyway.
As for SHIFT_COUNT_TRUNCATED, it should have been applied already from the
caller - expand_shift_1.

> >2016-02-15  Jakub Jelinek  <jakub@redhat.com>
> >
> >	PR rtl-optimization/69764
> >	PR rtl-optimization/69771
> >	* optabs.c (expand_binop): Ensure for shift optabs invalid CONST_INT
> >	op1 is valid for mode.
> >
> >--- gcc/optabs.c.jj	2016-02-12 17:49:25.000000000 +0100
> >+++ gcc/optabs.c	2016-02-15 16:15:53.983673792 +0100
> >@@ -1125,6 +1125,12 @@ expand_binop (machine_mode mode, optab b
> >       op1 = negate_rtx (mode, op1);
> >       binoptab = add_optab;
> >     }
> >+  /* For shifts, constant invalid op1 might be expanded from different
> >+     mode than MODE.  */
> >+  else if (CONST_INT_P (op1)
> >+	   && shift_optab_p (binoptab)
> >+	   && UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode)))
> >+    op1 = gen_int_mode (INTVAL (op1), mode);
> > 
> >   /* Record where to delete back to if we backtrack.  */
> >   last = get_last_insn ();

	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Richard Biener]

On February 15, 2016 7:15:35 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Mon, Feb 15, 2016 at 06:58:45PM +0100, Richard Biener wrote:
>> We could also force_reg those at expansion or apply
>SHIFT_COUNT_TRUNCATED to those invalid constants there.
>
>Sure, but for force_reg we'd still need the gen_int_mode anyway.
>As for SHIFT_COUNT_TRUNCATED, it should have been applied already from
>the
>caller - expand_shift_1.

But then no out of bound values should remain.  Until we get 256bit ints where your workaround wouldn't work either?

Richard.

>> >2016-02-15  Jakub Jelinek  <jakub@redhat.com>
>> >
>> >	PR rtl-optimization/69764
>> >	PR rtl-optimization/69771
>> >	* optabs.c (expand_binop): Ensure for shift optabs invalid
>CONST_INT
>> >	op1 is valid for mode.
>> >
>> >--- gcc/optabs.c.jj	2016-02-12 17:49:25.000000000 +0100
>> >+++ gcc/optabs.c	2016-02-15 16:15:53.983673792 +0100
>> >@@ -1125,6 +1125,12 @@ expand_binop (machine_mode mode, optab b
>> >       op1 = negate_rtx (mode, op1);
>> >       binoptab = add_optab;
>> >     }
>> >+  /* For shifts, constant invalid op1 might be expanded from
>different
>> >+     mode than MODE.  */
>> >+  else if (CONST_INT_P (op1)
>> >+	   && shift_optab_p (binoptab)
>> >+	   && UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode)))
>> >+    op1 = gen_int_mode (INTVAL (op1), mode);
>> > 
>> >   /* Record where to delete back to if we backtrack.  */
>> >   last = get_last_insn ();
>
>	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Jakub Jelinek]

On Mon, Feb 15, 2016 at 08:43:22PM +0100, Richard Biener wrote:
> On February 15, 2016 7:15:35 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
> >On Mon, Feb 15, 2016 at 06:58:45PM +0100, Richard Biener wrote:
> >> We could also force_reg those at expansion or apply
> >SHIFT_COUNT_TRUNCATED to those invalid constants there.
> >
> >Sure, but for force_reg we'd still need the gen_int_mode anyway.
> >As for SHIFT_COUNT_TRUNCATED, it should have been applied already from
> >the
> >caller - expand_shift_1.
> 
> But then no out of bound values should remain.
> Until we get 256bit ints where your workaround wouldn't work either?

Of course it would work, because in that case mode would be OImode, not
QImode, and thus the code would ensure the shift count is valid OImode
constant.

Anyway, the patch I've posted has been broken for vector shifts,
the last argument to gen_int_mode should have been GET_MODE_INNER (mode).

Here is a variant of that patch with force_reg, seems to work on
aarch64 and x86_64.

2016-02-16  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/69764
	PR rtl-optimization/69771
	* optabs.c (expand_binop): Ensure for shift optabs invalid CONST_INT
	op1 is valid for GET_MODE_INNER (mode) and force it into a reg.

--- gcc/optabs.c.jj	2016-02-15 22:22:46.161674598 +0100
+++ gcc/optabs.c	2016-02-16 08:20:01.206889067 +0100
@@ -1125,6 +1125,16 @@ expand_binop (machine_mode mode, optab b
       op1 = negate_rtx (mode, op1);
       binoptab = add_optab;
     }
+  /* For shifts, constant invalid op1 might be expanded from different
+     mode than MODE.  As those are invalid, force them to a register
+     to avoid further problems during expansion.  */
+  else if (CONST_INT_P (op1)
+	   && shift_optab_p (binoptab)
+	   && UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode)))
+    {
+      op1 = gen_int_mode (INTVAL (op1), GET_MODE_INNER (mode));
+      op1 = force_reg (GET_MODE_INNER (mode), op1);
+    }
 
   /* Record where to delete back to if we backtrack.  */
   last = get_last_insn ();


	Jakub

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Richard Biener]

On Tue, 16 Feb 2016, Jakub Jelinek wrote:

> On Mon, Feb 15, 2016 at 08:43:22PM +0100, Richard Biener wrote:
> > On February 15, 2016 7:15:35 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
> > >On Mon, Feb 15, 2016 at 06:58:45PM +0100, Richard Biener wrote:
> > >> We could also force_reg those at expansion or apply
> > >SHIFT_COUNT_TRUNCATED to those invalid constants there.
> > >
> > >Sure, but for force_reg we'd still need the gen_int_mode anyway.
> > >As for SHIFT_COUNT_TRUNCATED, it should have been applied already from
> > >the
> > >caller - expand_shift_1.
> > 
> > But then no out of bound values should remain.
> > Until we get 256bit ints where your workaround wouldn't work either?
> 
> Of course it would work, because in that case mode would be OImode, not
> QImode, and thus the code would ensure the shift count is valid OImode
> constant.

Ah, yes - here we're still using op0 mode.  For OImode wouldn't we
get a CONST_DOUBLE or CONST_WIDE_INT though?

> Anyway, the patch I've posted has been broken for vector shifts,
> the last argument to gen_int_mode should have been GET_MODE_INNER (mode).
>
> Here is a variant of that patch with force_reg, seems to work on
> aarch64 and x86_64.

Works for me.

Thanks,
Richard.

> 2016-02-16  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/69764
> 	PR rtl-optimization/69771
> 	* optabs.c (expand_binop): Ensure for shift optabs invalid CONST_INT
> 	op1 is valid for GET_MODE_INNER (mode) and force it into a reg.
> 
> --- gcc/optabs.c.jj	2016-02-15 22:22:46.161674598 +0100
> +++ gcc/optabs.c	2016-02-16 08:20:01.206889067 +0100
> @@ -1125,6 +1125,16 @@ expand_binop (machine_mode mode, optab b
>        op1 = negate_rtx (mode, op1);
>        binoptab = add_optab;
>      }
> +  /* For shifts, constant invalid op1 might be expanded from different
> +     mode than MODE.  As those are invalid, force them to a register
> +     to avoid further problems during expansion.  */
> +  else if (CONST_INT_P (op1)
> +	   && shift_optab_p (binoptab)
> +	   && UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode)))
> +    {
> +      op1 = gen_int_mode (INTVAL (op1), GET_MODE_INNER (mode));
> +      op1 = force_reg (GET_MODE_INNER (mode), op1);
> +    }
>  
>    /* Record where to delete back to if we backtrack.  */
>    last = get_last_insn ();
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion

[Jakub Jelinek]

On Tue, Feb 16, 2016 at 10:05:55AM +0100, Richard Biener wrote:
> On Tue, 16 Feb 2016, Jakub Jelinek wrote:
> 
> > On Mon, Feb 15, 2016 at 08:43:22PM +0100, Richard Biener wrote:
> > > On February 15, 2016 7:15:35 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
> > > >On Mon, Feb 15, 2016 at 06:58:45PM +0100, Richard Biener wrote:
> > > >> We could also force_reg those at expansion or apply
> > > >SHIFT_COUNT_TRUNCATED to those invalid constants there.
> > > >
> > > >Sure, but for force_reg we'd still need the gen_int_mode anyway.
> > > >As for SHIFT_COUNT_TRUNCATED, it should have been applied already from
> > > >the
> > > >caller - expand_shift_1.
> > > 
> > > But then no out of bound values should remain.
> > > Until we get 256bit ints where your workaround wouldn't work either?
> > 
> > Of course it would work, because in that case mode would be OImode, not
> > QImode, and thus the code would ensure the shift count is valid OImode
> > constant.
> 
> Ah, yes - here we're still using op0 mode.  For OImode wouldn't we
> get a CONST_DOUBLE or CONST_WIDE_INT though?

I think the C/C++ FEs use unsigned int for the type of the shift count in
any case, so op1 will still be CONST_INT.

> > Here is a variant of that patch with force_reg, seems to work on
> > aarch64 and x86_64.
> 
> Works for me.

Ok, will bootstrap/regtest it today and commit if it passes testing.

	Jakub