| Message ID | CAAgBjMnNe4YRPfKFO8XKoReQJ_Vg0VD9kthUE4gFPFjE5BTWJw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR90723 | expand |
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;
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;
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;
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;
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;
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.