This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PR90723


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;

Attachment: pr90723-3.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]