Prathamesh Kulkarni <prathamesh.kulka...@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.kulka...@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;
