From: Richard Biener <rguenther@suse.de>
To: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
Cc: Richard Biener <richard.guenther@gmail.com>,
gcc Patches <gcc-patches@gcc.gnu.org>,
Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org>,
Jim Wilson <jim.wilson@linaro.org>
Subject: Re: RFC [1/2] divmod transform
Date: Tue, 24 May 2016 12:19:00 -0000 [thread overview]
Message-ID: <alpine.LSU.2.11.1605241357590.18037@t29.fhfr.qr> (raw)
In-Reply-To: <CAAgBjM=hEZb6BnTMDL7KQ=78xGZ4D8osWyXysBedfroVec7BWg@mail.gmail.com>
On Tue, 24 May 2016, Prathamesh Kulkarni wrote:
> On 23 May 2016 at 17:35, Richard Biener <richard.guenther@gmail.com> wrote:
> > On Mon, May 23, 2016 at 10:58 AM, Prathamesh Kulkarni
> > <prathamesh.kulkarni@linaro.org> wrote:
> >> Hi,
> >> I have updated my patch for divmod (attached), which was originally
> >> based on Kugan's patch.
> >> The patch transforms stmts with code TRUNC_DIV_EXPR and TRUNC_MOD_EXPR
> >> having same operands to divmod representation, so we can cse computation of mod.
> >>
> >> t1 = a TRUNC_DIV_EXPR b;
> >> t2 = a TRUNC_MOD_EXPR b
> >> is transformed to:
> >> complex_tmp = DIVMOD (a, b);
> >> t1 = REALPART_EXPR (complex_tmp);
> >> t2 = IMAGPART_EXPR (complex_tmp);
> >>
> >> * New hook divmod_expand_libfunc
> >> The rationale for introducing the hook is that different targets have
> >> incompatible calling conventions for divmod libfunc.
> >> Currently three ports define divmod libfunc: c6x, spu and arm.
> >> c6x and spu follow the convention of libgcc2.c:__udivmoddi4:
> >> return quotient and store remainder in argument passed as pointer,
> >> while the arm version takes two arguments and returns both
> >> quotient and remainder having mode double the size of the operand mode.
> >> The port should hence override the hook expand_divmod_libfunc
> >> to generate call to target-specific divmod.
> >> Ports should define this hook if:
> >> a) The port does not have divmod or div insn for the given mode.
> >> b) The port defines divmod libfunc for the given mode.
> >> The default hook default_expand_divmod_libfunc() generates call
> >> to libgcc2.c:__udivmoddi4 provided the operands are unsigned and
> >> are of DImode.
> >>
> >> Patch passes bootstrap+test on x86_64-unknown-linux-gnu and
> >> cross-tested on arm*-*-*.
> >> Bootstrap+test in progress on arm-linux-gnueabihf.
> >> Does this patch look OK ?
> >
> > diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> > index 6b4601b..e4a021a 100644
> > --- a/gcc/targhooks.c
> > +++ b/gcc/targhooks.c
> > @@ -1965,4 +1965,31 @@ default_optab_supported_p (int, machine_mode,
> > machine_mode, optimization_type)
> > return true;
> > }
> >
> > +void
> > +default_expand_divmod_libfunc (bool unsignedp, machine_mode mode,
> > + rtx op0, rtx op1,
> > + rtx *quot_p, rtx *rem_p)
> >
> > functions need a comment.
> >
> > ISTR it was suggested that ARM change to libgcc2.c__udivmoddi4 style? In that
> > case we could avoid the target hook.
> Well I would prefer adding the hook because that's more easier -;)
> Would it be ok for now to go with the hook ?
> >
> > + /* If target overrides expand_divmod_libfunc hook
> > + then perform divmod by generating call to the target-specifc divmod
> > libfunc. */
> > + if (targetm.expand_divmod_libfunc != default_expand_divmod_libfunc)
> > + return true;
> > +
> > + /* Fall back to using libgcc2.c:__udivmoddi4. */
> > + return (mode == DImode && unsignedp);
> >
> > I don't understand this - we know optab_libfunc returns non-NULL for 'mode'
> > but still restrict this to DImode && unsigned? Also if
> > targetm.expand_divmod_libfunc
> > is not the default we expect the target to handle all modes?
> Ah indeed, the check for DImode is unnecessary.
> However I suppose the check for unsignedp should be there,
> since we want to generate call to __udivmoddi4 only if operand is unsigned ?
The optab libfunc for sdivmod should be NULL in that case.
> >
> > That said - I expected the above piece to be simply a 'return true;' ;)
> >
> > Usually we use some can_expand_XXX helper in optabs.c to query if the target
> > supports a specific operation (for example SImode divmod would use DImode
> > divmod by means of widening operands - for the unsigned case of course).
> Thanks for pointing out. So if a target does not support divmod
> libfunc for a mode
> but for a wider mode, then we could zero-extend operands to the wider-mode,
> perform divmod on the wider-mode, and then cast result back to the
> original mode.
> I haven't done that in this patch, would it be OK to do that as a follow up ?
I think that you should conservatively handle the div_optab query, thus if
the target has a HW division in a wider mode don't use the divmod IFN.
You'd simply iterate over GET_MODE_WIDER_MODE and repeat the
if (optab_handler (div_optab, mode) != CODE_FOR_nothing) check, bailing
out if that is available.
> > + /* Disable the transform if either is a constant, since
> > division-by-constant
> > + may have specialized expansion. */
> > + if (TREE_CONSTANT (op1) || TREE_CONSTANT (op2))
> > + return false;
> >
> > please use CONSTANT_CLASS_P (op1) || CONSTANT_CLASS_P (op2)
> >
> > + if (TYPE_OVERFLOW_TRAPS (type))
> > + return false;
> >
> > why's that? Generally please first test cheap things (trapping, constant-ness)
> > before checking expensive stuff (target_supports_divmod_p).
> I added TYPE_OVERFLOW_TRAPS (type) based on your suggestion in:
> https://www.mail-archive.com/gcc@gcc.gnu.org/msg78534.html
> "When looking at TRUNC_DIV_EXPR you should also exclude
> the case where TYPE_OVERFLOW_TRAPS (type) as that should
> expand using the [su]divv optabs (no trapping overflow
> divmod optab exists)."
Ok, didn't remember that.
> >
> > +static bool
> > +convert_to_divmod (gassign *stmt)
> > +{
> > + if (!divmod_candidate_p (stmt))
> > + return false;
> > +
> > + tree op1 = gimple_assign_rhs1 (stmt);
> > + tree op2 = gimple_assign_rhs2 (stmt);
> > +
> > + vec<gimple *> stmts = vNULL;
> >
> > use an auto_vec <gimple *> - you currently leak it in at least one place.
> >
> > + if (maybe_clean_or_replace_eh_stmt (use_stmt, use_stmt))
> > + cfg_changed = true;
> >
> > note that this suggests you should check whether any of the stmts may throw
> > internally as you don't update / transfer EH info correctly. So for 'stmt' and
> > all 'use_stmt' check stmt_can_throw_internal and if so do not add it to
> > the list of stmts to modify.
> >
> > Btw, I think you should not add 'stmt' immediately but when iterating over
> > all uses also gather uses in TRUNC_MOD_EXPR.
> >
> > Otherwise looks ok.
> Done changes in this version. I am gathering mod uses same time as div uses,
> so this imposes a constraint that mod dominates mod. I am not sure if
> that's desirable.
I think you also need a mod_seen variable now that you don't necessarily
end up with 'stmt' in the vector of stmts. I don't see how there is a
constraint that mod dominates mod - it's just that the top_stmt needs
to dominate all other uses that can be replaced with replacing top_stmt
with a divmod. It's just that the actual stmt set we choose may now
depend on immediate uses order which on a second thought is bad
as immediate uses order could be affected by debug stmts ... hmm.
To avoid this please re-add the code adding 'stmt' to stmts immediately
and add a use_stmt != stmt check to the immediate use processing loop
so that we don't end up adding it twice.
Richard.
next prev parent reply other threads:[~2016-05-24 12:14 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-23 8:58 Prathamesh Kulkarni
2016-05-23 12:05 ` Richard Biener
2016-05-24 12:08 ` Prathamesh Kulkarni
2016-05-24 12:19 ` Richard Biener [this message]
2016-05-24 14:52 ` Prathamesh Kulkarni
2016-05-24 14:59 ` Richard Biener
2016-05-24 16:50 ` Prathamesh Kulkarni
2016-05-25 9:20 ` Richard Biener
2016-05-25 13:33 ` Prathamesh Kulkarni
2016-05-27 12:05 ` Richard Biener
2016-05-27 12:41 ` Prathamesh Kulkarni
2016-05-27 13:04 ` Richard Biener
2016-05-30 9:56 ` Prathamesh Kulkarni
2016-05-30 10:36 ` Richard Biener
2016-05-31 14:20 ` Prathamesh Kulkarni
2016-06-01 11:09 ` Richard Biener
2016-06-03 21:10 ` Joseph Myers
2016-06-03 23:31 ` Jim Wilson
2016-06-08 14:23 ` Richard Biener
2016-07-28 13:36 ` Prathamesh Kulkarni
2016-08-09 10:54 ` Prathamesh Kulkarni
2016-08-13 11:26 ` Prathamesh Kulkarni
2016-08-13 11:43 ` Prathamesh Kulkarni
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=alpine.LSU.2.11.1605241357590.18037@t29.fhfr.qr \
--to=rguenther@suse.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=jim.wilson@linaro.org \
--cc=kugan.vivekanandarajah@linaro.org \
--cc=prathamesh.kulkarni@linaro.org \
--cc=ramana.radhakrishnan@arm.com \
--cc=richard.guenther@gmail.com \
/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).