PR90723

Message ID CAAgBjMnNe4YRPfKFO8XKoReQJ_Vg0VD9kthUE4gFPFjE5BTWJw@mail.gmail.com
State New
Headers show
Series
  • PR90723
Related show

Commit Message

Prathamesh Kulkarni July 9, 2019, 6:36 a.m.
Hi,
For following test-case:

typedef double v4df __attribute__ ((vector_size (32)));
void foo(v4df);

int
main ()
{
  volatile v4df x1;
  x1 = (v4df) { 10.0, 20.0, 30.0, 40.0 };
  foo (x1);
  return 0;
}

Compiling with -msve-vector-bits=256, the compiler goes into infinite
recursion and eventually segfaults due to stack overflow.

This happens during expansion of:
  x1.0_1 ={v} x1;

aarch64_expand_sve_mem_move calls aarch64_emit_sve_pred_move with
dest = (reg:VNx2DF 93) and
src = (mem/u/c:VNx2DF
           (plus:DI (reg/f:DI 94) (const_int 32 [0x20])) [0  S32 A128])

aarch64_emit_sve_pred_move calls expand_insn with above ops.
Eventually we hit EXPAND_INPUT case in maybe_legitimize_operand for
src (opno == 2)

Since the operand is marked with volatile, and volatile_ok is set to false,
insn_operand_matches return false and we call:
      op->value = copy_to_mode_reg (mode, op->value);
      break;

copy_to_mode_reg however, creates a fresh register and calls emit_move_insn:
rtx temp = gen_reg_rtx (mode);
if (x != temp)
    emit_move_insn (temp, x);

and we again end up in aarch64_emit_sve_pred_move, with dest assigned
the new register and src remaining unchanged, and thus the cycle continues.

As suggested by Richard, the attached patch temporarily sets volatile_ok to true
using RAII class volatile_ok_temp in aarch64_emit_sve_pred_move which
avoids the recursion.

Bootstrap + tested on x86_64-unknown-linux-gnu, aarch64-linux-gnu.
Cross-testing with SVE in progress.
OK to commit ?

Thanks,
Prathamesh
2019-07-09  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	PR target/90723
	* recog.h (volatile_ok_temp): New class.
	* config/aarch64/aarch64.c (aarch64_emit_sve_pred_move): Set
	volatile_ok temporarily to true using volatile_ok_temp.
	* expr.c (emit_block_move_via_cpymem): Likewise.
	* optabs.c (maybe_legitimize_operand): Likewise.

Comments

Richard Sandiford July 10, 2019, 8:18 p.m. | #1
Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> Hi,

> For following test-case:

>

> typedef double v4df __attribute__ ((vector_size (32)));

> void foo(v4df);

>

> int

> main ()

> {

>   volatile v4df x1;

>   x1 = (v4df) { 10.0, 20.0, 30.0, 40.0 };

>   foo (x1);

>   return 0;

> }

>

> Compiling with -msve-vector-bits=256, the compiler goes into infinite

> recursion and eventually segfaults due to stack overflow.

>

> This happens during expansion of:

>   x1.0_1 ={v} x1;

>

> aarch64_expand_sve_mem_move calls aarch64_emit_sve_pred_move with

> dest = (reg:VNx2DF 93) and

> src = (mem/u/c:VNx2DF

>            (plus:DI (reg/f:DI 94) (const_int 32 [0x20])) [0  S32 A128])

>

> aarch64_emit_sve_pred_move calls expand_insn with above ops.

> Eventually we hit EXPAND_INPUT case in maybe_legitimize_operand for

> src (opno == 2)

>

> Since the operand is marked with volatile, and volatile_ok is set to false,

> insn_operand_matches return false and we call:

>       op->value = copy_to_mode_reg (mode, op->value);

>       break;

>

> copy_to_mode_reg however, creates a fresh register and calls emit_move_insn:

> rtx temp = gen_reg_rtx (mode);

> if (x != temp)

>     emit_move_insn (temp, x);

>

> and we again end up in aarch64_emit_sve_pred_move, with dest assigned

> the new register and src remaining unchanged, and thus the cycle continues.

>

> As suggested by Richard, the attached patch temporarily sets volatile_ok to true

> using RAII class volatile_ok_temp in aarch64_emit_sve_pred_move which

> avoids the recursion.

>

> Bootstrap + tested on x86_64-unknown-linux-gnu, aarch64-linux-gnu.

> Cross-testing with SVE in progress.

> OK to commit ?

>

> Thanks,

> Prathamesh

>

> 2019-07-09  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

>

> 	PR target/90723

> 	* recog.h (volatile_ok_temp): New class.

> 	* config/aarch64/aarch64.c (aarch64_emit_sve_pred_move): Set

> 	volatile_ok temporarily to true using volatile_ok_temp.

> 	* expr.c (emit_block_move_via_cpymem): Likewise.

> 	* optabs.c (maybe_legitimize_operand): Likewise. 


I'm the last person who should be suggesting names, but how about
temporary_volatile_ok?  Putting "temp" after the name IMO makes it
sound too much like it's describing the RAII object rather than the
value of volatile_ok itself.

> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

> index 5a923ca006b..50afba1a2b6 100644

> --- a/gcc/config/aarch64/aarch64.c

> +++ b/gcc/config/aarch64/aarch64.c

> @@ -3457,6 +3457,7 @@ aarch64_emit_sve_pred_move (rtx dest, rtx pred, rtx src)

>    create_output_operand (&ops[0], dest, mode);

>    create_input_operand (&ops[1], pred, GET_MODE(pred));

>    create_input_operand (&ops[2], src, mode);

> +  volatile_ok_temp v(true);


Missing space before "(".  Same for later uses.

> [...]

> diff --git a/gcc/recog.h b/gcc/recog.h

> index 75cbbdc10ad..8a8eaf7e0c3 100644

> --- a/gcc/recog.h

> +++ b/gcc/recog.h

> @@ -186,6 +186,22 @@ skip_alternative (const char *p)

>  /* Nonzero means volatile operands are recognized.  */

>  extern int volatile_ok;

>  

> +/* RAII class for temporarily setting volatile_ok.  */

> +

> +class volatile_ok_temp

> +{

> +public:

> +  volatile_ok_temp(int value): save_volatile_ok (volatile_ok)


Missing space before "(" and before ":".

> +  {

> +    volatile_ok = value;

> +  }

> +

> +  ~volatile_ok_temp() { volatile_ok = save_volatile_ok; }


Missing space before "(".

> +

> +private:

> +  int save_volatile_ok;


For extra safety we should probably have a private copy constructor
(declaration only, no implementation).  We're still C++03 and so can't
use "= delete".

Thanks,
Richard


> +};

> +

>  /* Set by constrain_operands to the number of the alternative that

>     matched.  */

>  extern int which_alternative;
Prathamesh Kulkarni July 11, 2019, 7:57 a.m. | #2
On Thu, 11 Jul 2019 at 01:48, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>

> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:

> > Hi,

> > For following test-case:

> >

> > typedef double v4df __attribute__ ((vector_size (32)));

> > void foo(v4df);

> >

> > int

> > main ()

> > {

> >   volatile v4df x1;

> >   x1 = (v4df) { 10.0, 20.0, 30.0, 40.0 };

> >   foo (x1);

> >   return 0;

> > }

> >

> > Compiling with -msve-vector-bits=256, the compiler goes into infinite

> > recursion and eventually segfaults due to stack overflow.

> >

> > This happens during expansion of:

> >   x1.0_1 ={v} x1;

> >

> > aarch64_expand_sve_mem_move calls aarch64_emit_sve_pred_move with

> > dest = (reg:VNx2DF 93) and

> > src = (mem/u/c:VNx2DF

> >            (plus:DI (reg/f:DI 94) (const_int 32 [0x20])) [0  S32 A128])

> >

> > aarch64_emit_sve_pred_move calls expand_insn with above ops.

> > Eventually we hit EXPAND_INPUT case in maybe_legitimize_operand for

> > src (opno == 2)

> >

> > Since the operand is marked with volatile, and volatile_ok is set to false,

> > insn_operand_matches return false and we call:

> >       op->value = copy_to_mode_reg (mode, op->value);

> >       break;

> >

> > copy_to_mode_reg however, creates a fresh register and calls emit_move_insn:

> > rtx temp = gen_reg_rtx (mode);

> > if (x != temp)

> >     emit_move_insn (temp, x);

> >

> > and we again end up in aarch64_emit_sve_pred_move, with dest assigned

> > the new register and src remaining unchanged, and thus the cycle continues.

> >

> > As suggested by Richard, the attached patch temporarily sets volatile_ok to true

> > using RAII class volatile_ok_temp in aarch64_emit_sve_pred_move which

> > avoids the recursion.

> >

> > Bootstrap + tested on x86_64-unknown-linux-gnu, aarch64-linux-gnu.

> > Cross-testing with SVE in progress.

> > OK to commit ?

> >

> > Thanks,

> > Prathamesh

> >

> > 2019-07-09  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

> >

> >       PR target/90723

> >       * recog.h (volatile_ok_temp): New class.

> >       * config/aarch64/aarch64.c (aarch64_emit_sve_pred_move): Set

> >       volatile_ok temporarily to true using volatile_ok_temp.

> >       * expr.c (emit_block_move_via_cpymem): Likewise.

> >       * optabs.c (maybe_legitimize_operand): Likewise.

>

> I'm the last person who should be suggesting names, but how about

> temporary_volatile_ok?  Putting "temp" after the name IMO makes it

> sound too much like it's describing the RAII object rather than the

> value of volatile_ok itself.

>

> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

> > index 5a923ca006b..50afba1a2b6 100644

> > --- a/gcc/config/aarch64/aarch64.c

> > +++ b/gcc/config/aarch64/aarch64.c

> > @@ -3457,6 +3457,7 @@ aarch64_emit_sve_pred_move (rtx dest, rtx pred, rtx src)

> >    create_output_operand (&ops[0], dest, mode);

> >    create_input_operand (&ops[1], pred, GET_MODE(pred));

> >    create_input_operand (&ops[2], src, mode);

> > +  volatile_ok_temp v(true);

>

> Missing space before "(".  Same for later uses.

>

> > [...]

> > diff --git a/gcc/recog.h b/gcc/recog.h

> > index 75cbbdc10ad..8a8eaf7e0c3 100644

> > --- a/gcc/recog.h

> > +++ b/gcc/recog.h

> > @@ -186,6 +186,22 @@ skip_alternative (const char *p)

> >  /* Nonzero means volatile operands are recognized.  */

> >  extern int volatile_ok;

> >

> > +/* RAII class for temporarily setting volatile_ok.  */

> > +

> > +class volatile_ok_temp

> > +{

> > +public:

> > +  volatile_ok_temp(int value): save_volatile_ok (volatile_ok)

>

> Missing space before "(" and before ":".

>

> > +  {

> > +    volatile_ok = value;

> > +  }

> > +

> > +  ~volatile_ok_temp() { volatile_ok = save_volatile_ok; }

>

> Missing space before "(".

>

> > +

> > +private:

> > +  int save_volatile_ok;

>

> For extra safety we should probably have a private copy constructor

> (declaration only, no implementation).  We're still C++03 and so can't

> use "= delete".

Thanks for the suggestions.
Does the attached version look OK ?

Thanks,
Prathamesh
>

> Thanks,

> Richard

>

>

> > +};

> > +

> >  /* Set by constrain_operands to the number of the alternative that

> >     matched.  */

> >  extern int which_alternative;
2019-07-11  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	PR target/90723
	* recog.h (temporary_volatile_ok): New class.
	* config/aarch64/aarch64.c (aarch64_emit_sve_pred_move): Set
	volatile_ok temporarily to true using temporary_volatile_ok.
	* expr.c (emit_block_move_via_cpymem): Likewise.
	* optabs.c (maybe_legitimize_operand): Likewise.

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 5a923ca006b..abdf4b1c87a 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3457,6 +3457,7 @@ aarch64_emit_sve_pred_move (rtx dest, rtx pred, rtx src)
   create_output_operand (&ops[0], dest, mode);
   create_input_operand (&ops[1], pred, GET_MODE(pred));
   create_input_operand (&ops[2], src, mode);
+  temporary_volatile_ok v (true);
   expand_insn (code_for_aarch64_pred_mov (mode), 3, ops);
 }
 
diff --git a/gcc/expr.c b/gcc/expr.c
index 4acf250dd3c..795a56d88a6 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -1732,8 +1732,6 @@ emit_block_move_via_cpymem (rtx x, rtx y, rtx size, unsigned int align,
 			    unsigned HOST_WIDE_INT max_size,
 			    unsigned HOST_WIDE_INT probable_max_size)
 {
-  int save_volatile_ok = volatile_ok;
-
   if (expected_align < align)
     expected_align = align;
   if (expected_size != -1)
@@ -1745,7 +1743,7 @@ emit_block_move_via_cpymem (rtx x, rtx y, rtx size, unsigned int align,
     }
 
   /* Since this is a move insn, we don't care about volatility.  */
-  volatile_ok = 1;
+  temporary_volatile_ok v (true);
 
   /* Try the most limited insn first, because there's no point
      including more than one in the machine description unless
@@ -1809,14 +1807,10 @@ emit_block_move_via_cpymem (rtx x, rtx y, rtx size, unsigned int align,
 		create_fixed_operand (&ops[8], NULL);
 	    }
 	  if (maybe_expand_insn (code, nops, ops))
-	    {
-	      volatile_ok = save_volatile_ok;
-	      return true;
-	    }
+	    return true;
 	}
     }
 
-  volatile_ok = save_volatile_ok;
   return false;
 }
 
diff --git a/gcc/optabs.c b/gcc/optabs.c
index 18ca7370917..187a3d3621d 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -7202,17 +7202,15 @@ maybe_legitimize_operand (enum insn_code icode, unsigned int opno,
 			  struct expand_operand *op)
 {
   machine_mode mode, imode;
-  bool old_volatile_ok, result;
 
   mode = op->mode;
   switch (op->type)
     {
     case EXPAND_FIXED:
-      old_volatile_ok = volatile_ok;
-      volatile_ok = true;
-      result = maybe_legitimize_operand_same_code (icode, opno, op);
-      volatile_ok = old_volatile_ok;
-      return result;
+      {
+	temporary_volatile_ok v (true);
+	return maybe_legitimize_operand_same_code (icode, opno, op);
+      }
 
     case EXPAND_OUTPUT:
       gcc_assert (mode != VOIDmode);
diff --git a/gcc/recog.h b/gcc/recog.h
index 75cbbdc10ad..8a8c9125ff2 100644
--- a/gcc/recog.h
+++ b/gcc/recog.h
@@ -186,6 +186,23 @@ skip_alternative (const char *p)
 /* Nonzero means volatile operands are recognized.  */
 extern int volatile_ok;
 
+/* RAII class for temporarily setting volatile_ok.  */
+
+class temporary_volatile_ok
+{
+public:
+  temporary_volatile_ok (int value): save_volatile_ok (volatile_ok)
+  {
+    volatile_ok = value;
+  }
+
+  ~temporary_volatile_ok () { volatile_ok = save_volatile_ok; }
+
+private:
+  temporary_volatile_ok (const temporary_volatile_ok&);
+  int save_volatile_ok;
+};
+
 /* Set by constrain_operands to the number of the alternative that
    matched.  */
 extern int which_alternative;
Richard Sandiford July 11, 2019, 8:09 a.m. | #3
Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> @@ -186,6 +186,23 @@ skip_alternative (const char *p)

>  /* Nonzero means volatile operands are recognized.  */

>  extern int volatile_ok;

>  

> +/* RAII class for temporarily setting volatile_ok.  */

> +

> +class temporary_volatile_ok

> +{

> +public:

> +  temporary_volatile_ok (int value): save_volatile_ok (volatile_ok)


Missing space before the ":".

> +  {

> +    volatile_ok = value;

> +  }

> +

> +  ~temporary_volatile_ok () { volatile_ok = save_volatile_ok; }

> +

> +private:

> +  temporary_volatile_ok (const temporary_volatile_ok&);


Missing space before the "&".

OK with those changes, thanks.

Richard


> +  int save_volatile_ok;

> +};

> +

>  /* Set by constrain_operands to the number of the alternative that

>     matched.  */

>  extern int which_alternative;
Prathamesh Kulkarni July 13, 2019, 8:29 a.m. | #4
On Thu, 11 Jul 2019 at 13:39, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>

> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:

> > @@ -186,6 +186,23 @@ skip_alternative (const char *p)

> >  /* Nonzero means volatile operands are recognized.  */

> >  extern int volatile_ok;

> >

> > +/* RAII class for temporarily setting volatile_ok.  */

> > +

> > +class temporary_volatile_ok

> > +{

> > +public:

> > +  temporary_volatile_ok (int value): save_volatile_ok (volatile_ok)

>

> Missing space before the ":".

>

> > +  {

> > +    volatile_ok = value;

> > +  }

> > +

> > +  ~temporary_volatile_ok () { volatile_ok = save_volatile_ok; }

> > +

> > +private:

> > +  temporary_volatile_ok (const temporary_volatile_ok&);

>

> Missing space before the "&".

Oops, sorry about that.
>

> OK with those changes, thanks.

Thanks, committed as r273466.

Thanks,
Prathamesh
>

> Richard

>

>

> > +  int save_volatile_ok;

> > +};

> > +

> >  /* Set by constrain_operands to the number of the alternative that

> >     matched.  */

> >  extern int which_alternative;

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 5a923ca006b..50afba1a2b6 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3457,6 +3457,7 @@  aarch64_emit_sve_pred_move (rtx dest, rtx pred, rtx src)
   create_output_operand (&ops[0], dest, mode);
   create_input_operand (&ops[1], pred, GET_MODE(pred));
   create_input_operand (&ops[2], src, mode);
+  volatile_ok_temp v(true);
   expand_insn (code_for_aarch64_pred_mov (mode), 3, ops);
 }
 
diff --git a/gcc/expr.c b/gcc/expr.c
index 4acf250dd3c..87720875864 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -1732,8 +1732,6 @@  emit_block_move_via_cpymem (rtx x, rtx y, rtx size, unsigned int align,
 			    unsigned HOST_WIDE_INT max_size,
 			    unsigned HOST_WIDE_INT probable_max_size)
 {
-  int save_volatile_ok = volatile_ok;
-
   if (expected_align < align)
     expected_align = align;
   if (expected_size != -1)
@@ -1745,7 +1743,7 @@  emit_block_move_via_cpymem (rtx x, rtx y, rtx size, unsigned int align,
     }
 
   /* Since this is a move insn, we don't care about volatility.  */
-  volatile_ok = 1;
+  volatile_ok_temp v(true);
 
   /* Try the most limited insn first, because there's no point
      including more than one in the machine description unless
@@ -1809,14 +1807,10 @@  emit_block_move_via_cpymem (rtx x, rtx y, rtx size, unsigned int align,
 		create_fixed_operand (&ops[8], NULL);
 	    }
 	  if (maybe_expand_insn (code, nops, ops))
-	    {
-	      volatile_ok = save_volatile_ok;
-	      return true;
-	    }
+	    return true;
 	}
     }
 
-  volatile_ok = save_volatile_ok;
   return false;
 }
 
diff --git a/gcc/optabs.c b/gcc/optabs.c
index 18ca7370917..1959087b7b3 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -7202,17 +7202,15 @@  maybe_legitimize_operand (enum insn_code icode, unsigned int opno,
 			  struct expand_operand *op)
 {
   machine_mode mode, imode;
-  bool old_volatile_ok, result;
 
   mode = op->mode;
   switch (op->type)
     {
     case EXPAND_FIXED:
-      old_volatile_ok = volatile_ok;
-      volatile_ok = true;
-      result = maybe_legitimize_operand_same_code (icode, opno, op);
-      volatile_ok = old_volatile_ok;
-      return result;
+      {
+	volatile_ok_temp v(true);
+	return maybe_legitimize_operand_same_code (icode, opno, op);
+      }
 
     case EXPAND_OUTPUT:
       gcc_assert (mode != VOIDmode);
diff --git a/gcc/recog.h b/gcc/recog.h
index 75cbbdc10ad..8a8eaf7e0c3 100644
--- a/gcc/recog.h
+++ b/gcc/recog.h
@@ -186,6 +186,22 @@  skip_alternative (const char *p)
 /* Nonzero means volatile operands are recognized.  */
 extern int volatile_ok;
 
+/* RAII class for temporarily setting volatile_ok.  */
+
+class volatile_ok_temp
+{
+public:
+  volatile_ok_temp(int value): save_volatile_ok (volatile_ok)
+  {
+    volatile_ok = value;
+  }
+
+  ~volatile_ok_temp() { volatile_ok = save_volatile_ok; }
+
+private:
+  int save_volatile_ok;
+};
+
 /* Set by constrain_operands to the number of the alternative that
    matched.  */
 extern int which_alternative;