From: Richard Biener <rguenther@suse.de>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Jeff Law <law@redhat.com>,
gcc-patches@gcc.gnu.org, Thomas Koenig <tkoenig@netcologne.de>
Subject: Re: [PATCH] expansion: FIx up infinite recusion due to double-word modulo optimization
Date: Wed, 2 Dec 2020 08:53:45 +0100 (CET) [thread overview]
Message-ID: <nycvar.YFH.7.76.2012020853340.17979@zhemvz.fhfr.qr> (raw)
In-Reply-To: <20201201210713.GH3788@tucnak>
On Tue, 1 Dec 2020, Jakub Jelinek wrote:
> Hi!
>
> Jeff has reported that my earlier patch broke rl78-elf, e.g. with
> unsigned short foo (unsigned short x) { return x % 7; }
> when compiled with -O2 -mg14. The problem is that rl78 is a BITS_PER_WORD
> == 8 target which doesn't have 8-bit modulo or divmod optab, but has instead
> 16-bit divmod, so my patch attempted to optimize it, then called
> expand_divmod to do 8-bit modulo and that in turn tried to do 16-bit modulo
> again.
>
> The following patch fixes it in two ways.
> One is to not perform the optimization when we have {u,s}divmod_optab
> handler for the double-word mode, in that case it is IMHO better to just
> do whatever we used to do before. This alone should fix the infinite
> recursion. But I'd be afraid some other target might have similar problem
> and might not have a divmod pattern, but only say a library call.
> So the patch also introduces a methods argument to expand_divmod such that
> normally we allow everything that was allowed before (using libcalls and
> widening), but when called from these expand_doubleword*mod routines we
> restrict it to no widening and no libcalls.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Thanks,
Richard.
> 2020-12-01 Jakub Jelinek <jakub@redhat.com>
>
> * expmed.h (expand_divmod): Only declare if GCC_OPTABS_H is defined.
> Add enum optabs_method argument defaulted to OPTAB_LIB_WIDEN.
> * expmed.c: Include expmed.h after optabs.h.
> (expand_divmod): Add methods argument, if it is not OPTAB_{,LIB_}WIDEN,
> don't choose a wider mode, and pass it to other calls instead of
> hardcoded OPTAB_LIB_WIDEN. Avoid emitting libcalls if not
> OPTAB_LIB or OPTAB_LIB_WIDEN.
> * optabs.c: Include expmed.h after optabs.h.
> (expand_doubleword_mod, expand_doubleword_divmod): Pass OPTAB_DIRECT
> as last argument to expand_divmod.
> (expand_binop): Punt if {s,u}divmod_optab has handler for double-word
> int_mode.
> * expr.c: Include expmed.h after optabs.h.
> * explow.c: Include expmed.h after optabs.h.
>
> --- gcc/expmed.h.jj 2020-01-12 11:54:36.565411115 +0100
> +++ gcc/expmed.h 2020-12-01 19:25:55.117128688 +0100
> @@ -716,8 +716,10 @@ extern rtx expand_variable_shift (enum t
> rtx, tree, rtx, int);
> extern rtx expand_shift (enum tree_code, machine_mode, rtx, poly_int64, rtx,
> int);
> +#ifdef GCC_OPTABS_H
> extern rtx expand_divmod (int, enum tree_code, machine_mode, rtx, rtx,
> - rtx, int);
> + rtx, int, enum optab_methods = OPTAB_LIB_WIDEN);
> +#endif
> #endif
>
> extern void store_bit_field (rtx, poly_uint64, poly_uint64,
> --- gcc/expmed.c.jj 2020-08-10 10:44:20.659560695 +0200
> +++ gcc/expmed.c 2020-12-01 19:30:40.993941266 +0100
> @@ -31,8 +31,8 @@ along with GCC; see the file COPYING3.
> #include "predict.h"
> #include "memmodel.h"
> #include "tm_p.h"
> -#include "expmed.h"
> #include "optabs.h"
> +#include "expmed.h"
> #include "regs.h"
> #include "emit-rtl.h"
> #include "diagnostic-core.h"
> @@ -4193,7 +4193,8 @@ expand_sdiv_pow2 (scalar_int_mode mode,
>
> rtx
> expand_divmod (int rem_flag, enum tree_code code, machine_mode mode,
> - rtx op0, rtx op1, rtx target, int unsignedp)
> + rtx op0, rtx op1, rtx target, int unsignedp,
> + enum optab_methods methods)
> {
> machine_mode compute_mode;
> rtx tquotient;
> @@ -4299,17 +4300,22 @@ expand_divmod (int rem_flag, enum tree_c
> optab2 = (op1_is_pow2 ? optab1
> : (unsignedp ? udivmod_optab : sdivmod_optab));
>
> - FOR_EACH_MODE_FROM (compute_mode, mode)
> - if (optab_handler (optab1, compute_mode) != CODE_FOR_nothing
> - || optab_handler (optab2, compute_mode) != CODE_FOR_nothing)
> - break;
> -
> - if (compute_mode == VOIDmode)
> - FOR_EACH_MODE_FROM (compute_mode, mode)
> - if (optab_libfunc (optab1, compute_mode)
> - || optab_libfunc (optab2, compute_mode))
> + if (methods == OPTAB_WIDEN || methods == OPTAB_LIB_WIDEN)
> + {
> + FOR_EACH_MODE_FROM (compute_mode, mode)
> + if (optab_handler (optab1, compute_mode) != CODE_FOR_nothing
> + || optab_handler (optab2, compute_mode) != CODE_FOR_nothing)
> break;
>
> + if (compute_mode == VOIDmode && methods == OPTAB_LIB_WIDEN)
> + FOR_EACH_MODE_FROM (compute_mode, mode)
> + if (optab_libfunc (optab1, compute_mode)
> + || optab_libfunc (optab2, compute_mode))
> + break;
> + }
> + else
> + compute_mode = mode;
> +
> /* If we still couldn't find a mode, use MODE, but expand_binop will
> probably die. */
> if (compute_mode == VOIDmode)
> @@ -4412,8 +4418,7 @@ expand_divmod (int rem_flag, enum tree_c
> remainder
> = expand_binop (int_mode, and_optab, op0,
> gen_int_mode (mask, int_mode),
> - remainder, 1,
> - OPTAB_LIB_WIDEN);
> + remainder, 1, methods);
> if (remainder)
> return gen_lowpart (mode, remainder);
> }
> @@ -4721,7 +4726,7 @@ expand_divmod (int rem_flag, enum tree_c
> remainder = expand_binop
> (int_mode, and_optab, op0,
> gen_int_mode (mask, int_mode),
> - remainder, 0, OPTAB_LIB_WIDEN);
> + remainder, 0, methods);
> if (remainder)
> return gen_lowpart (mode, remainder);
> }
> @@ -4846,7 +4851,7 @@ expand_divmod (int rem_flag, enum tree_c
> do_cmp_and_jump (op1, const0_rtx, LT, compute_mode, label2);
> do_cmp_and_jump (adjusted_op0, const0_rtx, LT, compute_mode, label1);
> tem = expand_binop (compute_mode, sdiv_optab, adjusted_op0, op1,
> - quotient, 0, OPTAB_LIB_WIDEN);
> + quotient, 0, methods);
> if (tem != quotient)
> emit_move_insn (quotient, tem);
> emit_jump_insn (targetm.gen_jump (label5));
> @@ -4858,7 +4863,7 @@ expand_divmod (int rem_flag, enum tree_c
> emit_label (label2);
> do_cmp_and_jump (adjusted_op0, const0_rtx, GT, compute_mode, label3);
> tem = expand_binop (compute_mode, sdiv_optab, adjusted_op0, op1,
> - quotient, 0, OPTAB_LIB_WIDEN);
> + quotient, 0, methods);
> if (tem != quotient)
> emit_move_insn (quotient, tem);
> emit_jump_insn (targetm.gen_jump (label5));
> @@ -4867,7 +4872,7 @@ expand_divmod (int rem_flag, enum tree_c
> expand_dec (adjusted_op0, const1_rtx);
> emit_label (label4);
> tem = expand_binop (compute_mode, sdiv_optab, adjusted_op0, op1,
> - quotient, 0, OPTAB_LIB_WIDEN);
> + quotient, 0, methods);
> if (tem != quotient)
> emit_move_insn (quotient, tem);
> expand_dec (quotient, const1_rtx);
> @@ -4892,7 +4897,7 @@ expand_divmod (int rem_flag, enum tree_c
> floor_log2 (d), tquotient, 1);
> t2 = expand_binop (int_mode, and_optab, op0,
> gen_int_mode (d - 1, int_mode),
> - NULL_RTX, 1, OPTAB_LIB_WIDEN);
> + NULL_RTX, 1, methods);
> t3 = gen_reg_rtx (int_mode);
> t3 = emit_store_flag (t3, NE, t2, const0_rtx, int_mode, 1, 1);
> if (t3 == 0)
> @@ -4963,7 +4968,7 @@ expand_divmod (int rem_flag, enum tree_c
> emit_label (label1);
> expand_dec (adjusted_op0, const1_rtx);
> tem = expand_binop (compute_mode, udiv_optab, adjusted_op0, op1,
> - quotient, 1, OPTAB_LIB_WIDEN);
> + quotient, 1, methods);
> if (tem != quotient)
> emit_move_insn (quotient, tem);
> expand_inc (quotient, const1_rtx);
> @@ -4987,7 +4992,7 @@ expand_divmod (int rem_flag, enum tree_c
> floor_log2 (d), tquotient, 0);
> t2 = expand_binop (compute_mode, and_optab, op0,
> gen_int_mode (d - 1, compute_mode),
> - NULL_RTX, 1, OPTAB_LIB_WIDEN);
> + NULL_RTX, 1, methods);
> t3 = gen_reg_rtx (compute_mode);
> t3 = emit_store_flag (t3, NE, t2, const0_rtx,
> compute_mode, 1, 1);
> @@ -5063,7 +5068,7 @@ expand_divmod (int rem_flag, enum tree_c
> do_cmp_and_jump (adjusted_op0, const0_rtx, GT,
> compute_mode, label1);
> tem = expand_binop (compute_mode, sdiv_optab, adjusted_op0, op1,
> - quotient, 0, OPTAB_LIB_WIDEN);
> + quotient, 0, methods);
> if (tem != quotient)
> emit_move_insn (quotient, tem);
> emit_jump_insn (targetm.gen_jump (label5));
> @@ -5076,7 +5081,7 @@ expand_divmod (int rem_flag, enum tree_c
> do_cmp_and_jump (adjusted_op0, const0_rtx, LT,
> compute_mode, label3);
> tem = expand_binop (compute_mode, sdiv_optab, adjusted_op0, op1,
> - quotient, 0, OPTAB_LIB_WIDEN);
> + quotient, 0, methods);
> if (tem != quotient)
> emit_move_insn (quotient, tem);
> emit_jump_insn (targetm.gen_jump (label5));
> @@ -5085,7 +5090,7 @@ expand_divmod (int rem_flag, enum tree_c
> expand_inc (adjusted_op0, const1_rtx);
> emit_label (label4);
> tem = expand_binop (compute_mode, sdiv_optab, adjusted_op0, op1,
> - quotient, 0, OPTAB_LIB_WIDEN);
> + quotient, 0, methods);
> if (tem != quotient)
> emit_move_insn (quotient, tem);
> expand_inc (quotient, const1_rtx);
> @@ -5133,10 +5138,10 @@ expand_divmod (int rem_flag, enum tree_c
> {
> rtx tem;
> quotient = expand_binop (int_mode, udiv_optab, op0, op1,
> - quotient, 1, OPTAB_LIB_WIDEN);
> + quotient, 1, methods);
> tem = expand_mult (int_mode, quotient, op1, NULL_RTX, 1);
> remainder = expand_binop (int_mode, sub_optab, op0, tem,
> - remainder, 1, OPTAB_LIB_WIDEN);
> + remainder, 1, methods);
> }
> tem = plus_constant (int_mode, op1, -1);
> tem = expand_shift (RSHIFT_EXPR, int_mode, tem, 1, NULL_RTX, 1);
> @@ -5158,10 +5163,10 @@ expand_divmod (int rem_flag, enum tree_c
> {
> rtx tem;
> quotient = expand_binop (int_mode, sdiv_optab, op0, op1,
> - quotient, 0, OPTAB_LIB_WIDEN);
> + quotient, 0, methods);
> tem = expand_mult (int_mode, quotient, op1, NULL_RTX, 0);
> remainder = expand_binop (int_mode, sub_optab, op0, tem,
> - remainder, 0, OPTAB_LIB_WIDEN);
> + remainder, 0, methods);
> }
> abs_rem = expand_abs (int_mode, remainder, NULL_RTX, 1, 0);
> abs_op1 = expand_abs (int_mode, op1, NULL_RTX, 1, 0);
> @@ -5258,7 +5263,7 @@ expand_divmod (int rem_flag, enum tree_c
> quotient = sign_expand_binop (compute_mode,
> udiv_optab, sdiv_optab,
> op0, op1, target,
> - unsignedp, OPTAB_LIB_WIDEN);
> + unsignedp, methods);
> }
> }
> }
> @@ -5273,10 +5278,11 @@ expand_divmod (int rem_flag, enum tree_c
> /* No divide instruction either. Use library for remainder. */
> remainder = sign_expand_binop (compute_mode, umod_optab, smod_optab,
> op0, op1, target,
> - unsignedp, OPTAB_LIB_WIDEN);
> + unsignedp, methods);
> /* No remainder function. Try a quotient-and-remainder
> function, keeping the remainder. */
> - if (!remainder)
> + if (!remainder
> + && (methods == OPTAB_LIB || methods == OPTAB_LIB_WIDEN))
> {
> remainder = gen_reg_rtx (compute_mode);
> if (!expand_twoval_binop_libfunc
> @@ -5294,10 +5300,14 @@ expand_divmod (int rem_flag, enum tree_c
> NULL_RTX, unsignedp);
> remainder = expand_binop (compute_mode, sub_optab, op0,
> remainder, target, unsignedp,
> - OPTAB_LIB_WIDEN);
> + methods);
> }
> }
>
> + if (methods != OPTAB_LIB_WIDEN
> + && (rem_flag ? remainder : quotient) == NULL_RTX)
> + return NULL_RTX;
> +
> return gen_lowpart (mode, rem_flag ? remainder : quotient);
> }
> \f
> --- gcc/optabs.c.jj 2020-12-01 19:02:52.251555958 +0100
> +++ gcc/optabs.c 2020-12-01 19:26:25.069794805 +0100
> @@ -28,8 +28,8 @@ along with GCC; see the file COPYING3.
> #include "memmodel.h"
> #include "predict.h"
> #include "tm_p.h"
> -#include "expmed.h"
> #include "optabs.h"
> +#include "expmed.h"
> #include "emit-rtl.h"
> #include "recog.h"
> #include "diagnostic-core.h"
> @@ -1082,7 +1082,7 @@ expand_doubleword_mod (machine_mode mode
> }
> }
> rtx remainder = expand_divmod (1, TRUNC_MOD_EXPR, word_mode, sum, op1,
> - NULL_RTX, 1);
> + NULL_RTX, 1, OPTAB_DIRECT);
> if (remainder == NULL_RTX)
> return NULL_RTX;
>
> @@ -1180,7 +1180,7 @@ expand_doubleword_divmod (machine_mode m
> if (op11 != const1_rtx)
> {
> rtx rem2 = expand_divmod (1, TRUNC_MOD_EXPR, mode, quot1, op11,
> - NULL_RTX, unsignedp);
> + NULL_RTX, unsignedp, OPTAB_DIRECT);
> if (rem2 == NULL_RTX)
> return NULL_RTX;
>
> @@ -1195,7 +1195,7 @@ expand_doubleword_divmod (machine_mode m
> return NULL_RTX;
>
> rtx quot2 = expand_divmod (0, TRUNC_DIV_EXPR, mode, quot1, op11,
> - NULL_RTX, unsignedp);
> + NULL_RTX, unsignedp, OPTAB_DIRECT);
> if (quot2 == NULL_RTX)
> return NULL_RTX;
>
> @@ -2100,6 +2100,9 @@ expand_binop (machine_mode mode, optab b
> && CONST_INT_P (op1)
> && is_int_mode (mode, &int_mode)
> && GET_MODE_SIZE (int_mode) == 2 * UNITS_PER_WORD
> + && optab_handler ((binoptab == umod_optab || binoptab == udiv_optab)
> + ? udivmod_optab : sdivmod_optab,
> + int_mode) == CODE_FOR_nothing
> && optab_handler (and_optab, word_mode) != CODE_FOR_nothing
> && optab_handler (add_optab, word_mode) != CODE_FOR_nothing
> && optimize_insn_for_speed_p ())
> --- gcc/expr.c.jj 2020-11-22 19:11:44.088588689 +0100
> +++ gcc/expr.c 2020-12-01 19:26:45.521566815 +0100
> @@ -29,8 +29,8 @@ along with GCC; see the file COPYING3.
> #include "memmodel.h"
> #include "tm_p.h"
> #include "ssa.h"
> -#include "expmed.h"
> #include "optabs.h"
> +#include "expmed.h"
> #include "regs.h"
> #include "emit-rtl.h"
> #include "recog.h"
> --- gcc/explow.c.jj 2020-11-26 01:14:47.500082297 +0100
> +++ gcc/explow.c 2020-12-01 19:25:19.944520777 +0100
> @@ -27,9 +27,9 @@ along with GCC; see the file COPYING3.
> #include "tree.h"
> #include "memmodel.h"
> #include "tm_p.h"
> +#include "optabs.h"
> #include "expmed.h"
> #include "profile-count.h"
> -#include "optabs.h"
> #include "emit-rtl.h"
> #include "recog.h"
> #include "diagnostic-core.h"
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
prev parent reply other threads:[~2020-12-02 7:53 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-01 21:07 Jakub Jelinek
2020-12-01 22:37 ` Jeff Law
2020-12-02 7:53 ` Richard Biener [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.YFH.7.76.2012020853340.17979@zhemvz.fhfr.qr \
--to=rguenther@suse.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=law@redhat.com \
--cc=tkoenig@netcologne.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).