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.
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)
