On Fri, 21 Mar 2025 at 11:39, Richard Biener <rguent...@suse.de> wrote:
>
> On Fri, 21 Mar 2025, Richard Earnshaw wrote:
>
> > If expand_binop_directly fails to add a REG_EQUAL note it tries to
> > unwind and restart. But it can unwind too far if expand_binop changed
> > some of the operands before calling it. We don't need to unwind that
> > far anyway since we should end up taking exactly the same route next
> > time, just without a target rtx.
> >
> > To fix this we remove LAST from the argument list and let the callers
> > (all in expand_binop) do their own unwinding if the call fails.
> > Instead we unwind just as far as the entry to expand_binop_directly
> > and recurse within this function instead of all the way back up.
>
> This looks good and like a nice cleanup as well. But please give
> others more familiar with this code the chance to chime in.
>
I was running a bootstrap on arm-linux-gnueabihf with the patch Andrew
attached in bugzilla. It passes bootstrap and make check, I think it
applies to this patch as well, as it is very similar.
Thanks,
Christophe
> Thanks,
> Richard.
>
> > gcc/ChangeLog:
> >
> > PR middle-end/117811
> > * optabs.cc (expand_binop_directly): Remove LAST as an argument,
> > instead record the last insn on entry. Only delete insns if
> > we need to restart and restart by calling ourself, not expand_binop.
> > (expand_binop): Update callers to expand_binop_directly. If it
> > fails to expand the operation, delete back to LAST.
> >
> > gcc/testsuite:
> >
> > PR middle-end/117811
> > * gcc.dg/torture/pr117811.c: New test.
> > ---
> > gcc/optabs.cc | 24 +++++++++++-----------
> > gcc/testsuite/gcc.dg/torture/pr117811.c | 27 +++++++++++++++++++++++++
> > 2 files changed, 39 insertions(+), 12 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.dg/torture/pr117811.c
> >
> > diff --git a/gcc/optabs.cc b/gcc/optabs.cc
> > index 36f2e6af8b5..0a14b1eef8a 100644
> > --- a/gcc/optabs.cc
> > +++ b/gcc/optabs.cc
> > @@ -1369,8 +1369,7 @@ avoid_expensive_constant (machine_mode mode, optab
> > binoptab,
> > static rtx
> > expand_binop_directly (enum insn_code icode, machine_mode mode, optab
> > binoptab,
> > rtx op0, rtx op1,
> > - rtx target, int unsignedp, enum optab_methods methods,
> > - rtx_insn *last)
> > + rtx target, int unsignedp, enum optab_methods methods)
> > {
> > machine_mode xmode0 = insn_data[(int) icode].operand[1].mode;
> > machine_mode xmode1 = insn_data[(int) icode].operand[2].mode;
> > @@ -1380,6 +1379,7 @@ expand_binop_directly (enum insn_code icode,
> > machine_mode mode, optab binoptab,
> > rtx_insn *pat;
> > rtx xop0 = op0, xop1 = op1;
> > bool canonicalize_op1 = false;
> > + rtx_insn *last = get_last_insn ();
> >
> > /* If it is a commutative operator and the modes would match
> > if we would swap the operands, we can save the conversions. */
> > @@ -1444,10 +1444,7 @@ expand_binop_directly (enum insn_code icode,
> > machine_mode mode, optab binoptab,
> > tmp_mode = insn_data[(int) icode].operand[0].mode;
> > if (VECTOR_MODE_P (mode)
> > && maybe_ne (GET_MODE_NUNITS (tmp_mode), 2 * GET_MODE_NUNITS
> > (mode)))
> > - {
> > - delete_insns_since (last);
> > - return NULL_RTX;
> > - }
> > + return NULL_RTX;
> > }
> > else
> > tmp_mode = mode;
> > @@ -1467,14 +1464,14 @@ expand_binop_directly (enum insn_code icode,
> > machine_mode mode, optab binoptab,
> > ops[1].value, ops[2].value, mode0))
> > {
> > delete_insns_since (last);
> > - return expand_binop (mode, binoptab, op0, op1, NULL_RTX,
> > - unsignedp, methods);
> > + return expand_binop_directly (icode, mode, binoptab, op0, op1,
> > + NULL_RTX, unsignedp, methods);
> > }
> >
> > emit_insn (pat);
> > return ops[0].value;
> > }
> > - delete_insns_since (last);
> > +
> > return NULL_RTX;
> > }
> >
> > @@ -1543,9 +1540,10 @@ expand_binop (machine_mode mode, optab binoptab, rtx
> > op0, rtx op1,
> > if (icode != CODE_FOR_nothing)
> > {
> > temp = expand_binop_directly (icode, mode, binoptab, op0, op1,
> > - target, unsignedp, methods, last);
> > + target, unsignedp, methods);
> > if (temp)
> > return temp;
> > + delete_insns_since (last);
> > }
> > }
> >
> > @@ -1571,9 +1569,10 @@ expand_binop (machine_mode mode, optab binoptab, rtx
> > op0, rtx op1,
> > NULL_RTX, unsignedp, OPTAB_DIRECT);
> >
> > temp = expand_binop_directly (icode, int_mode, otheroptab, op0,
> > newop1,
> > - target, unsignedp, methods, last);
> > + target, unsignedp, methods);
> > if (temp)
> > return temp;
> > + delete_insns_since (last);
> > }
> >
> > /* If this is a multiply, see if we can do a widening operation that
> > @@ -1637,9 +1636,10 @@ expand_binop (machine_mode mode, optab binoptab, rtx
> > op0, rtx op1,
> > if (vop1)
> > {
> > temp = expand_binop_directly (icode, mode, otheroptab, op0,
> > vop1,
> > - target, unsignedp, methods, last);
> > + target, unsignedp, methods);
> > if (temp)
> > return temp;
> > + delete_insns_since (last);
> > }
> > }
> > }
> > diff --git a/gcc/testsuite/gcc.dg/torture/pr117811.c
> > b/gcc/testsuite/gcc.dg/torture/pr117811.c
> > new file mode 100644
> > index 00000000000..13d7e134780
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/torture/pr117811.c
> > @@ -0,0 +1,27 @@
> > +/* { dg-do run } */
> > +
> > +#include <string.h>
> > +
> > +typedef int v4 __attribute__((vector_size (4 * sizeof (int))));
> > +
> > +void __attribute__((noclone,noinline)) do_shift (v4 *vec, int shift)
> > +{
> > + v4 t = *vec;
> > +
> > + if (shift > 0)
> > + {
> > + t = t >> shift;
> > + }
> > +
> > + *vec = t;
> > +}
> > +
> > +int main ()
> > +{
> > + v4 vec = {0x1000000, 0x2000, 0x300, 0x40};
> > + v4 vec2 = {0x100000, 0x200, 0x30, 0x4};
> > + do_shift (&vec, 4);
> > + if (memcmp (&vec, &vec2, sizeof (v4)) != 0)
> > + __builtin_abort ();
> > + return 0;
> > +}
> >
>
> --
> Richard Biener <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH,
> Frankenstrasse 146, 90461 Nuernberg, Germany;
> GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
