Patchwork Fix iv_number_of_iterations (PR rtl-optimization/55838)

login
register
mail settings
Submitter Jakub Jelinek
Date Jan. 3, 2013, 8:34 a.m.
Message ID <20130103083455.GF7269@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/209194/
State New
Headers show

Comments

Jakub Jelinek - Jan. 3, 2013, 8:34 a.m.
Hi!

When one (or both) IVs have extend_mode wider than mode, but step doesn't
fit into mode (the IV is
(subreg:MODE (plus:EXTEND_MODE base (mult:EXTEND_MODE i step)) lowpart)
), such as for EXTEND_MODE SImode, MODE QImode and step e.g. 129, 128, 256
or 517, iv_number_of_iterations can create invalid rtl.  I think it is safe
to just use the lowpart subreg of the step.  The second hunk isn't enough,
we use iv0.step resp. iv1.step directly in several other places in the
routine, and the first hunk IMHO isn't enough either, if for the above
extend_mode SI and mode QI iv1.step is 128, the first hunk will make -128 out of
it, but then we negate it and get step 128 out of it again, not valid QImode
CONST_INT, and use it e.g. as argument to UMOD.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Or shall we just punt (either as done by H.J. in the PR, or in
iv_number_of_iterations if the steps aren't valid mode CONST_INTs)?

2013-01-03  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/55838
	* loop-iv.c (iv_number_of_iterations): Call lowpart_subreg on
	iv0.step, iv1.step and step.

	* gcc.dg/pr55838.c: New test.


	Jakub
Richard Guenther - Jan. 3, 2013, 8:55 a.m.
On Thu, 3 Jan 2013, Jakub Jelinek wrote:

> Hi!
> 
> When one (or both) IVs have extend_mode wider than mode, but step doesn't
> fit into mode (the IV is
> (subreg:MODE (plus:EXTEND_MODE base (mult:EXTEND_MODE i step)) lowpart)
> ), such as for EXTEND_MODE SImode, MODE QImode and step e.g. 129, 128, 256
> or 517, iv_number_of_iterations can create invalid rtl.  I think it is safe
> to just use the lowpart subreg of the step.  The second hunk isn't enough,
> we use iv0.step resp. iv1.step directly in several other places in the
> routine, and the first hunk IMHO isn't enough either, if for the above
> extend_mode SI and mode QI iv1.step is 128, the first hunk will make -128 out of
> it, but then we negate it and get step 128 out of it again, not valid QImode
> CONST_INT, and use it e.g. as argument to UMOD.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Truncating step is ok I think and so is the patch unless Zdenek
has any comments in the next few days.

Thanks,
Richard.

> Or shall we just punt (either as done by H.J. in the PR, or in
> iv_number_of_iterations if the steps aren't valid mode CONST_INTs)?
> 
> 2013-01-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/55838
> 	* loop-iv.c (iv_number_of_iterations): Call lowpart_subreg on
> 	iv0.step, iv1.step and step.
> 
> 	* gcc.dg/pr55838.c: New test.
> 
> --- gcc/loop-iv.c.jj	2012-10-22 08:42:25.000000000 +0200
> +++ gcc/loop-iv.c	2013-01-02 09:17:42.215591646 +0100
> @@ -2406,6 +2406,9 @@ iv_number_of_iterations (struct loop *lo
>        iv1.step = const0_rtx;
>      }
>  
> +  iv0.step = lowpart_subreg (mode, iv0.step, comp_mode);
> +  iv1.step = lowpart_subreg (mode, iv1.step, comp_mode);
> +
>    /* This is either infinite loop or the one that ends immediately, depending
>       on initial values.  Unswitching should remove this kind of conditions.  */
>    if (iv0.step == const0_rtx && iv1.step == const0_rtx)
> @@ -2516,6 +2519,7 @@ iv_number_of_iterations (struct loop *lo
>  	step = simplify_gen_unary (NEG, comp_mode, iv1.step, comp_mode);
>        else
>  	step = iv0.step;
> +      step = lowpart_subreg (mode, step, comp_mode);
>        delta = simplify_gen_binary (MINUS, comp_mode, iv1.base, iv0.base);
>        delta = lowpart_subreg (mode, delta, comp_mode);
>        delta = simplify_gen_binary (UMOD, mode, delta, step);
> --- gcc/testsuite/gcc.dg/pr55838.c.jj	2012-11-17 15:43:17.572007394 +0100
> +++ gcc/testsuite/gcc.dg/pr55838.c	2013-01-02 21:09:55.726580313 +0100
> @@ -0,0 +1,13 @@
> +/* PR rtl-optimization/55838 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -funroll-loops" } */
> +
> +int a;
> +unsigned char c;
> +
> +void
> +f (void)
> +{
> +  while (c++ < 2)
> +    c = a += 129;
> +}
> 
> 	Jakub
> 
>
Zdenek Dvorak - Jan. 3, 2013, 9:01 a.m.
Hi,

> When one (or both) IVs have extend_mode wider than mode, but step doesn't
> fit into mode (the IV is
> (subreg:MODE (plus:EXTEND_MODE base (mult:EXTEND_MODE i step)) lowpart)
> ), such as for EXTEND_MODE SImode, MODE QImode and step e.g. 129, 128, 256
> or 517, iv_number_of_iterations can create invalid rtl.  I think it is safe
> to just use the lowpart subreg of the step.  The second hunk isn't enough,
> we use iv0.step resp. iv1.step directly in several other places in the
> routine, and the first hunk IMHO isn't enough either, if for the above
> extend_mode SI and mode QI iv1.step is 128, the first hunk will make -128 out of
> it, but then we negate it and get step 128 out of it again, not valid QImode
> CONST_INT, and use it e.g. as argument to UMOD.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

I think this is ok,

Zdenek

Patch

--- gcc/loop-iv.c.jj	2012-10-22 08:42:25.000000000 +0200
+++ gcc/loop-iv.c	2013-01-02 09:17:42.215591646 +0100
@@ -2406,6 +2406,9 @@  iv_number_of_iterations (struct loop *lo
       iv1.step = const0_rtx;
     }
 
+  iv0.step = lowpart_subreg (mode, iv0.step, comp_mode);
+  iv1.step = lowpart_subreg (mode, iv1.step, comp_mode);
+
   /* This is either infinite loop or the one that ends immediately, depending
      on initial values.  Unswitching should remove this kind of conditions.  */
   if (iv0.step == const0_rtx && iv1.step == const0_rtx)
@@ -2516,6 +2519,7 @@  iv_number_of_iterations (struct loop *lo
 	step = simplify_gen_unary (NEG, comp_mode, iv1.step, comp_mode);
       else
 	step = iv0.step;
+      step = lowpart_subreg (mode, step, comp_mode);
       delta = simplify_gen_binary (MINUS, comp_mode, iv1.base, iv0.base);
       delta = lowpart_subreg (mode, delta, comp_mode);
       delta = simplify_gen_binary (UMOD, mode, delta, step);
--- gcc/testsuite/gcc.dg/pr55838.c.jj	2012-11-17 15:43:17.572007394 +0100
+++ gcc/testsuite/gcc.dg/pr55838.c	2013-01-02 21:09:55.726580313 +0100
@@ -0,0 +1,13 @@ 
+/* PR rtl-optimization/55838 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -funroll-loops" } */
+
+int a;
+unsigned char c;
+
+void
+f (void)
+{
+  while (c++ < 2)
+    c = a += 129;
+}