On 2 January 2017 at 20:37, Martin Liška wrote:
> On 10/16/2016 07:59 AM, Prathamesh Kulkarni wrote:
>> + /* Disable the transform if either is a constant, since
>> division-by-constant
>> + may have specialized expansion. */
>> + if (CONSTANT_CLASS_P (op1) || CONSTANT_CLASS_P (op2))
>> +return false;
>
> Hello.
>
> I've just played a bit with DIVMOD and realized that the DIVMOD optimization
> is
> disable when either op1 or op2 are constant. Aforementioned hunk contains a
> bit strange
> comment, where one is not sure that the condition is intentional?
Well, since expand_divmod() has specialized expansions for
division/mod for few constants,
the divmod transform is disabled if one of it's operands is constant.
I suppose the check could be made more precise by tracking cases that
are handled specially by expand_divmod,
but I didn't have a look at that, and simply disabled the transform if
one of the operands is a constant.
Thanks,
Prathamesh
>
> Thanks for clarification.
> Martin

On 10/16/2016 07:59 AM, Prathamesh Kulkarni wrote:
> + /* Disable the transform if either is a constant, since
> division-by-constant
> + may have specialized expansion. */
> + if (CONSTANT_CLASS_P (op1) || CONSTANT_CLASS_P (op2))
> +return false;
Hello.
I've just played a bit with DIVMOD and realized that the DIVMOD optimization is
disable when either op1 or op2 are constant. Aforementioned hunk contains a bit
strange
comment, where one is not sure that the condition is intentional?
Thanks for clarification.
Martin

On 25 October 2016 at 16:17, Richard Biener wrote:
> On Tue, 25 Oct 2016, Prathamesh Kulkarni wrote:
>
>> On 25 October 2016 at 13:43, Richard Biener
>> wrote:
>> > On Sun, Oct 16, 2016 at 7:59 AM, Prathamesh Kulkarni
>> > wrote:
>> >> Hi,
>> >> After approval from Bernd Schmidt, I committed the patch to remove
>> >> optab functions for
>> >> sdivmod_optab and udivmod_optab in optabs.def, which removes the block
>> >> for divmod patch.
>> >>
>> >> This patch is mostly the same as previous one, except it drops
>> >> targeting __udivmoddi4() because
>> >> it gave undefined reference link error for calling __udivmoddi4() on
>> >> aarch64-linux-gnu.
>> >> It appears aarch64 has hardware insn for DImode div, so __udivmoddi4()
>> >> isn't needed for the target
>> >> (it was a bug in my patch that called __udivmoddi4() even though
>> >> aarch64 supported hardware div).
>> >>
>> >> However this makes me wonder if it's guaranteed that __udivmoddi4()
>> >> will be available for a target if it doesn't have hardware div and
>> >> divmod insn and doesn't have target-specific libfunc for
>> >> DImode divmod ? To be conservative, the attached patch doesn't
>> >> generate call to __udivmoddi4.
>> >>
>> >> Passes bootstrap+test on x86_64-unknown-linux.
>> >> Cross-tested on arm*-*-*, aarch64*-*-*.
>> >> Verified that there are no regressions with SPEC2006 on
>> >> x86_64-unknown-linux-gnu.
>> >> OK to commit ?
>> >
>> > I think the searching is still somewhat wrong - it's been some time
>> > since my last look at the
>> > patch so maybe I've said this already. Please bail out early for
>> > stmt_can_throw_internal (stmt),
>> > otherwise the top stmt search might end up not working. So
>> >
>> > +
>> > + if (top_stmt == stmt && stmt_can_throw_internal (top_stmt))
>> > +return false;
>> >
>> > can go.
>> >
>> > top_stmt may end up as a TRUNC_DIV_EXPR so it's pointless to only look
>> > for another
>> > TRUNC_DIV_EXPR later ... you may end up without a single TRUNC_MOD_EXPR.
>> > Which means you want a div_seen and a mod_seen, or simply record the
>> > top_stmt
>> > code and look for the opposite in the 2nd loop.
>> Um sorry I don't quite understand how we could end up without a trunc_mod
>> stmt ?
>> The 2nd loop adds both trunc_div and trunc_mod to stmts vector, and
>> checks if we have
>> come across at least a single trunc_div stmt (and we bail out if no
>> div is seen).
>>
>> At 2nd loop I suppose we don't need mod_seen, because stmt is
>> guaranteed to be trunc_mod_expr.
>> In the 2nd loop the following condition will never trigger for stmt:
>> if (stmt_can_throw_internal (use_stmt))
>> continue;
>> since we checked before hand if stmt could throw and chose to bail out
>> in that case.
>>
>> and the following condition would also not trigger for stmt:
>> if (!dominated_by_p (CDI_DOMINATORS, gimple_bb (use_stmt), top_bb))
>> {
>> end_imm_use_stmt_traverse (_iter);
>> return false;
>> }
>> since gimple_bb (stmt) is always dominated by gimple_bb (top_stmt).
>>
>> The case where top_stmt == stmt, we wouldn't reach the above
>> condition, since we have above it:
>> if (top_stmt == stmt)
>> continue;
>>
>> So IIUC, top_stmt and stmt would always get added to stmts vector.
>> Am I missing something ?
>
> Ah, indeed. Maybe add a comment then, it wasn't really obvious ;)
>
> Please still move the stmt_can_throw_internal (stmt) check up.
Sure, I will move that up and do the other suggested changes.
I was wondering if this condition in 2nd loop is too restrictive ?
if (!dominated_by_p (CDI_DOMINATORS, gimple_bb (use_stmt), top_bb))
{
end_imm_use_stmt_traverse (_iter);
return false;
}
Should we rather "continue" in this case by not adding use_stmt to
stmts vector rather than dropping
the transform all-together if gimple_bb (use_stmt) is not dominated by
gimple_bb (top_stmt) ?
For instance if we have a test-case like:
if (cond)
{
t1 = x / y;
t2 = x % y;
}
else
t3 = x % y;
and suppose stmt is "t2 = x % y", we would set top_stmt to "t1 = x / y";
In this case we would still want to do divmod transform in THEN block
even though "t3 = x % y" is not dominated by top_stmt ?
if (cond)
{
divmod_tmp = DIVMOD (x, y);
t1 = REALPART_EXPR (divmod_tmp);
t2 = IMAGPART_EXPR (divmod_tmp);
}
else
t3 = x % y;
We will always ensure that all the trunc_div, trunc_mod statements in
stmts vector will be dominated by top_stmt,
but I suppose they need not constitute all the trunc_div, trunc_mod
statements in the function.
Thanks,
Prathamesh
>
> Thanks,
> Richard.
>
>> Thanks,
>> Prathamesh
>> >
>> > + switch (gimple_assign_rhs_code (use_stmt))
>> > + {
>> > + case TRUNC_DIV_EXPR:
>> > + new_rhs = fold_build1 (REALPART_EXPR, TREE_TYPE (op1), res);
>> > + break;
>> > +
>> > + case TRUNC_MOD_EXPR:
>> > + new_rhs = fold_build1 (IMAGPART_EXPR, TREE_TYPE

On 25 October 2016 at 13:43, Richard Biener wrote:
> On Sun, Oct 16, 2016 at 7:59 AM, Prathamesh Kulkarni
> wrote:
>> Hi,
>> After approval from Bernd Schmidt, I committed the patch to remove
>> optab functions for
>> sdivmod_optab and udivmod_optab in optabs.def, which removes the block
>> for divmod patch.
>>
>> This patch is mostly the same as previous one, except it drops
>> targeting __udivmoddi4() because
>> it gave undefined reference link error for calling __udivmoddi4() on
>> aarch64-linux-gnu.
>> It appears aarch64 has hardware insn for DImode div, so __udivmoddi4()
>> isn't needed for the target
>> (it was a bug in my patch that called __udivmoddi4() even though
>> aarch64 supported hardware div).
>>
>> However this makes me wonder if it's guaranteed that __udivmoddi4()
>> will be available for a target if it doesn't have hardware div and
>> divmod insn and doesn't have target-specific libfunc for
>> DImode divmod ? To be conservative, the attached patch doesn't
>> generate call to __udivmoddi4.
>>
>> Passes bootstrap+test on x86_64-unknown-linux.
>> Cross-tested on arm*-*-*, aarch64*-*-*.
>> Verified that there are no regressions with SPEC2006 on
>> x86_64-unknown-linux-gnu.
>> OK to commit ?
>
> I think the searching is still somewhat wrong - it's been some time
> since my last look at the
> patch so maybe I've said this already. Please bail out early for
> stmt_can_throw_internal (stmt),
> otherwise the top stmt search might end up not working. So
>
> +
> + if (top_stmt == stmt && stmt_can_throw_internal (top_stmt))
> +return false;
>
> can go.
>
> top_stmt may end up as a TRUNC_DIV_EXPR so it's pointless to only look
> for another
> TRUNC_DIV_EXPR later ... you may end up without a single TRUNC_MOD_EXPR.
> Which means you want a div_seen and a mod_seen, or simply record the top_stmt
> code and look for the opposite in the 2nd loop.
Um sorry I don't quite understand how we could end up without a trunc_mod stmt ?
The 2nd loop adds both trunc_div and trunc_mod to stmts vector, and
checks if we have
come across at least a single trunc_div stmt (and we bail out if no
div is seen).
At 2nd loop I suppose we don't need mod_seen, because stmt is
guaranteed to be trunc_mod_expr.
In the 2nd loop the following condition will never trigger for stmt:
if (stmt_can_throw_internal (use_stmt))
continue;
since we checked before hand if stmt could throw and chose to bail out
in that case.
and the following condition would also not trigger for stmt:
if (!dominated_by_p (CDI_DOMINATORS, gimple_bb (use_stmt), top_bb))
{
end_imm_use_stmt_traverse (_iter);
return false;
}
since gimple_bb (stmt) is always dominated by gimple_bb (top_stmt).
The case where top_stmt == stmt, we wouldn't reach the above
condition, since we have above it:
if (top_stmt == stmt)
continue;
So IIUC, top_stmt and stmt would always get added to stmts vector.
Am I missing something ?
Thanks,
Prathamesh
>
> + switch (gimple_assign_rhs_code (use_stmt))
> + {
> + case TRUNC_DIV_EXPR:
> + new_rhs = fold_build1 (REALPART_EXPR, TREE_TYPE (op1), res);
> + break;
> +
> + case TRUNC_MOD_EXPR:
> + new_rhs = fold_build1 (IMAGPART_EXPR, TREE_TYPE (op2), res);
> + break;
> +
>
> why type of op1 and type of op2 in the other case? Choose one for
> consistency.
>
> + if (maybe_clean_or_replace_eh_stmt (use_stmt, use_stmt))
> + cfg_changed = true;
>
> as you are rejecting all internally throwing stmts this shouldn't be
> necessary.
>
> The patch is ok with those changes.
>
> Thanks,
> Richard.
>
>
>> Thanks,
>> Prathamesh

On Sun, Oct 16, 2016 at 7:59 AM, Prathamesh Kulkarni
wrote:
> Hi,
> After approval from Bernd Schmidt, I committed the patch to remove
> optab functions for
> sdivmod_optab and udivmod_optab in optabs.def, which removes the block
> for divmod patch.
>
> This patch is mostly the same as previous one, except it drops
> targeting __udivmoddi4() because
> it gave undefined reference link error for calling __udivmoddi4() on
> aarch64-linux-gnu.
> It appears aarch64 has hardware insn for DImode div, so __udivmoddi4()
> isn't needed for the target
> (it was a bug in my patch that called __udivmoddi4() even though
> aarch64 supported hardware div).
>
> However this makes me wonder if it's guaranteed that __udivmoddi4()
> will be available for a target if it doesn't have hardware div and
> divmod insn and doesn't have target-specific libfunc for
> DImode divmod ? To be conservative, the attached patch doesn't
> generate call to __udivmoddi4.
>
> Passes bootstrap+test on x86_64-unknown-linux.
> Cross-tested on arm*-*-*, aarch64*-*-*.
> Verified that there are no regressions with SPEC2006 on
> x86_64-unknown-linux-gnu.
> OK to commit ?
I think the searching is still somewhat wrong - it's been some time
since my last look at the
patch so maybe I've said this already. Please bail out early for
stmt_can_throw_internal (stmt),
otherwise the top stmt search might end up not working. So
+
+ if (top_stmt == stmt && stmt_can_throw_internal (top_stmt))
+return false;
can go.
top_stmt may end up as a TRUNC_DIV_EXPR so it's pointless to only look
for another
TRUNC_DIV_EXPR later ... you may end up without a single TRUNC_MOD_EXPR.
Which means you want a div_seen and a mod_seen, or simply record the top_stmt
code and look for the opposite in the 2nd loop.
+ switch (gimple_assign_rhs_code (use_stmt))
+ {
+ case TRUNC_DIV_EXPR:
+ new_rhs = fold_build1 (REALPART_EXPR, TREE_TYPE (op1), res);
+ break;
+
+ case TRUNC_MOD_EXPR:
+ new_rhs = fold_build1 (IMAGPART_EXPR, TREE_TYPE (op2), res);
+ break;
+
why type of op1 and type of op2 in the other case? Choose one for consistency.
+ if (maybe_clean_or_replace_eh_stmt (use_stmt, use_stmt))
+ cfg_changed = true;
as you are rejecting all internally throwing stmts this shouldn't be necessary.
The patch is ok with those changes.
Thanks,
Richard.
> Thanks,
> Prathamesh

On 10/24/2016 09:35 AM, Prathamesh Kulkarni wrote:
On 24 October 2016 at 20:23, Jeff Law wrote:
On 10/24/2016 01:28 AM, Richard Biener wrote:
[ big snip ]
+
+ /* Update all statements in stmts.
+ if stmt is lhs = op1 TRUNC_DIV_EXPR op2, change to lhs =
REALPART_EXPR
+ if stmt is lhs = op1 TRUNC_MOD_EXPR op2, change to lhs =
IMAGPART_EXPR. */
I'd just emit a copy from RES to the appropriate lhs operand just
after
the
divmod and delete the now unnecessary TRUNC_DIV_EXPR and
TRUNC_MOD_EXPR
statements.
That sounds like a good idea.
Um sorry, not sure if I understood this part.
For eg:
t1 = x / y;
t2 = x % y;
do we want to transform it to:
complex_tmp = DIVMOD (x, y);
div_tmp = REALPART_EXPR
mod_tmp = IMAGPART_EXPR
complex_tmp = DIVMOD (x, y)
t1 = REALPART_EXPR (complex_tmp)
t2 = IMAGPART_EXPR (compex_tmp)
Then remove the
t1 = x/y
t2 = x%y
statements
OTOH implementation-wise that's more complicated.
If so, then I wouldn't bother. I only mention it because I've found that
model is sometimes easier on the implementation side. I don't consider it a
big deal.
Richard and Jeff,
Unless you have any further suggestions on the patch, should I
consider it approved (modulo formatting fixes) ?
I can confirm that the optab_libfunc() issue is solved at least for
the divmod transform with the patch to remove
optab functions for sdivmod_optab and udivmod_optab.
I think Richi has the most state on this series, so I'll let him chime in.
jeff

On 10/21/2016 04:34 AM, Prathamesh Kulkarni wrote:
On 20 October 2016 at 15:02, Richard Biener wrote:
On Wed, 19 Oct 2016, Jeff Law wrote:
On 10/15/2016 11:59 PM, Prathamesh Kulkarni wrote:
Hi,
After approval from Bernd Schmidt, I committed the patch to remove
optab functions for
sdivmod_optab and udivmod_optab in optabs.def, which removes the block
for divmod patch.
This patch is mostly the same as previous one, except it drops
targeting __udivmoddi4() because
it gave undefined reference link error for calling __udivmoddi4() on
aarch64-linux-gnu.
It appears aarch64 has hardware insn for DImode div, so __udivmoddi4()
isn't needed for the target
(it was a bug in my patch that called __udivmoddi4() even though
aarch64 supported hardware div).
However this makes me wonder if it's guaranteed that __udivmoddi4()
will be available for a target if it doesn't have hardware div and
divmod insn and doesn't have target-specific libfunc for
DImode divmod ? To be conservative, the attached patch doesn't
generate call to __udivmoddi4.
Passes bootstrap+test on x86_64-unknown-linux.
Cross-tested on arm*-*-*, aarch64*-*-*.
Verified that there are no regressions with SPEC2006 on
x86_64-unknown-linux-gnu.
OK to commit ?
Thanks,
Prathamesh
divmod-v2-3-main.txt
2016-10-15 Prathamesh Kulkarni
Kugan Vivekanandarajah
Jim Wilson
* target.def: New hook expand_divmod_libfunc.
* doc/tm.texi.in: Add hook for TARGET_EXPAND_DIVMOD_LIBFUNC
* doc/tm.texi: Regenerate.
* internal-fn.def: Add new entry for DIVMOD ifn.
* internal-fn.c (expand_DIVMOD): New.
* tree-ssa-math-opts.c: Include optabs-libfuncs.h, tree-eh.h,
targhooks.h.
(widen_mul_stats): Add new field divmod_calls_inserted.
(target_supports_divmod_p): New.
(divmod_candidate_p): Likewise.
(convert_to_divmod): Likewise.
(pass_optimize_widening_mul::execute): Call
calculate_dominance_info(), renumber_gimple_stmt_uids() at
beginning of function. Call convert_to_divmod()
and record stats for divmod.
Starting with some high level design comments. If these conflict with
comments from others, let me know and we'll work through the issues.
I don't really like introducing code conditional on the target capabilities
this early in the gimple optimization pipeline.
It's basically done right before RTL expansion
(pass_optimize_widening_mul).
Would it be possible to always do the transformation to divmod in the gimple
optimizers, regardless of the target capabilities. Then in the gimple->RTL
expanders make a final decision about divmod insn, libcall, or using div/mod
insns?
The issue is that it hoists one or both the division or the modulo and
if we don't do the transform we'd want to undo that code motion.
That would move all the target dependencies out of the gimple optimizers and
into the gimple->rtl expansion phase, which is the preferred place to start
introducing this kind of target dependency.
With that background, I'm going to focus more on the identification of divmod
opportunities than the expansion bits.
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index a4a8e49..866c368 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -7078,6 +7078,11 @@ This is firstly introduced on ARM/AArch64 targets,
please refer to
the hook implementation for how different fusion types are supported.
@end deftypefn
+@deftypefn {Target Hook} void TARGET_EXPAND_DIVMOD_LIBFUNC (rtx
@var{libfunc}, machine_mode @var{mode}, rtx @var{op0}, rtx @var{op1}, rtx
*@var{quot}, rtx *@var{rem})
+Define this hook for enabling divmod transform if the port does not have
+hardware divmod insn but defines target-specific divmod libfuncs.
+@end deftypefn
+
@node Sections
@section Dividing the Output into Sections (Texts, Data, @dots{})
@c the above section title is WAY too long. maybe cut the part between
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 265f1be..c4c387b 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -4890,6 +4890,8 @@ them: try the first ones in this list first.
@hook TARGET_SCHED_FUSION_PRIORITY
+@hook TARGET_EXPAND_DIVMOD_LIBFUNC
+
@node Sections
@section Dividing the Output into Sections (Texts, Data, @dots{})
@c the above section title is WAY too long. maybe cut the part between
diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c
index 0b32d5f..42c6973 100644
--- a/gcc/internal-fn.c
+++ b/gcc/internal-fn.c
@@ -2207,6 +2207,53 @@ expand_ATOMIC_COMPARE_EXCHANGE (internal_fn, gcall
*call)
expand_ifn_atomic_compare_exchange (call);
}
+/* Expand DIVMOD() using:
In general, we do not use () when referring to objects in comments.
+ a) optab handler for udivmod/sdivmod if it is available.
+ b) If optab_handler doesn't exist, generate call to
+target-specific divmod libfunc. */
+
+static void

On 10/20/2016 03:32 AM, Richard Biener wrote:
Starting with some high level design comments. If these conflict with
comments from others, let me know and we'll work through the issues.
I don't really like introducing code conditional on the target capabilities
this early in the gimple optimization pipeline.
It's basically done right before RTL expansion
(pass_optimize_widening_mul).
I didn't look at the placement of the pass as it hadn't changed.
Looking at that now, I see it's fairly late in the pipeline, so I won't
object to its placement.
Would it be possible to always do the transformation to divmod in the gimple
optimizers, regardless of the target capabilities. Then in the gimple->RTL
expanders make a final decision about divmod insn, libcall, or using div/mod
insns?
The issue is that it hoists one or both the division or the modulo and
if we don't do the transform we'd want to undo that code motion.
But don't we only hoist when we find a suitable div/mod pair to turn
into a divmod? Maybe I'm mis-understanding how the identification works.
[ snip ]
*/
+
+ FOR_EACH_IMM_USE_STMT (use_stmt, use_iter, op1)
+{
+ if (is_gimple_assign (use_stmt)
+ && (gimple_assign_rhs_code (use_stmt) == TRUNC_DIV_EXPR
+ || gimple_assign_rhs_code (use_stmt) == TRUNC_MOD_EXPR)
+ && operand_equal_p (op1, gimple_assign_rhs1 (use_stmt), 0)
+ && operand_equal_p (op2, gimple_assign_rhs2 (use_stmt), 0))
So why check for TRUNC_MOD_EXPR here? ISTM that stmt is always going to be
TRUNC_MOD_EXPR and thus you're only interested in looking for a matching
TRUNC_DIV_EXPR statement.
The only way I could see TRUNC_MOD_EXPR being useful here would be if there is
a redundant TRUNC_MOD_EXPR in the IL, which would be a sign that some other
optimizer hasn't done its job. Have you seen this to be useful in practice?
Note that I've reviewed this parts already and we restructured things
in this way, requiring to look for TRUNC_MOD_EXPR to properly handle
finding a dominating trunc _or_ div and interatively doing so
correctly if we have more than one pair.
But doesn't having more than one pair essentially mean that the other
optimziers have left redundant junk in the IL? I'm clearly missing
something here.
+
+ gimple_stmt_iterator gsi = gsi_for_stmt (use_stmt);
+ gimple_assign_set_rhs_from_tree (, new_rhs);
+ update_stmt (use_stmt);
+
+ if (maybe_clean_or_replace_eh_stmt (use_stmt, use_stmt))
+ cfg_changed = true;
Does this ever happen given how you filter out throwing statements earlier?
We filter out throwing stmts that are not "top". The top one we replace
with the divmod call and we can preserve EH for that (and thus handle
the case where the original div/mod at this place may throw).
OK. It just struck me as a bit odd. No worries if y'all have already
worked through this.
Jeff

On 10/15/2016 11:59 PM, Prathamesh Kulkarni wrote:
Hi,
After approval from Bernd Schmidt, I committed the patch to remove
optab functions for
sdivmod_optab and udivmod_optab in optabs.def, which removes the block
for divmod patch.
This patch is mostly the same as previous one, except it drops
targeting __udivmoddi4() because
it gave undefined reference link error for calling __udivmoddi4() on
aarch64-linux-gnu.
It appears aarch64 has hardware insn for DImode div, so __udivmoddi4()
isn't needed for the target
(it was a bug in my patch that called __udivmoddi4() even though
aarch64 supported hardware div).
However this makes me wonder if it's guaranteed that __udivmoddi4()
will be available for a target if it doesn't have hardware div and
divmod insn and doesn't have target-specific libfunc for
DImode divmod ? To be conservative, the attached patch doesn't
generate call to __udivmoddi4.
Passes bootstrap+test on x86_64-unknown-linux.
Cross-tested on arm*-*-*, aarch64*-*-*.
Verified that there are no regressions with SPEC2006 on
x86_64-unknown-linux-gnu.
OK to commit ?
Thanks,
Prathamesh
divmod-v2-3-main.txt
2016-10-15 Prathamesh Kulkarni
Kugan Vivekanandarajah
Jim Wilson
* target.def: New hook expand_divmod_libfunc.
* doc/tm.texi.in: Add hook for TARGET_EXPAND_DIVMOD_LIBFUNC
* doc/tm.texi: Regenerate.
* internal-fn.def: Add new entry for DIVMOD ifn.
* internal-fn.c (expand_DIVMOD): New.
* tree-ssa-math-opts.c: Include optabs-libfuncs.h, tree-eh.h,
targhooks.h.
(widen_mul_stats): Add new field divmod_calls_inserted.
(target_supports_divmod_p): New.
(divmod_candidate_p): Likewise.
(convert_to_divmod): Likewise.
(pass_optimize_widening_mul::execute): Call
calculate_dominance_info(), renumber_gimple_stmt_uids() at
beginning of function. Call convert_to_divmod()
and record stats for divmod.
Starting with some high level design comments. If these conflict with
comments from others, let me know and we'll work through the issues.
I don't really like introducing code conditional on the target
capabilities this early in the gimple optimization pipeline.
Would it be possible to always do the transformation to divmod in the
gimple optimizers, regardless of the target capabilities. Then in the
gimple->RTL expanders make a final decision about divmod insn, libcall,
or using div/mod insns?
That would move all the target dependencies out of the gimple optimizers
and into the gimple->rtl expansion phase, which is the preferred place
to start introducing this kind of target dependency.
With that background, I'm going to focus more on the identification of
divmod opportunities than the expansion bits.
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index a4a8e49..866c368 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -7078,6 +7078,11 @@ This is firstly introduced on ARM/AArch64 targets,
please refer to
the hook implementation for how different fusion types are supported.
@end deftypefn
+@deftypefn {Target Hook} void TARGET_EXPAND_DIVMOD_LIBFUNC (rtx @var{libfunc},
machine_mode @var{mode}, rtx @var{op0}, rtx @var{op1}, rtx *@var{quot}, rtx
*@var{rem})
+Define this hook for enabling divmod transform if the port does not have
+hardware divmod insn but defines target-specific divmod libfuncs.
+@end deftypefn
+
@node Sections
@section Dividing the Output into Sections (Texts, Data, @dots{})
@c the above section title is WAY too long. maybe cut the part between
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 265f1be..c4c387b 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -4890,6 +4890,8 @@ them: try the first ones in this list first.
@hook TARGET_SCHED_FUSION_PRIORITY
+@hook TARGET_EXPAND_DIVMOD_LIBFUNC
+
@node Sections
@section Dividing the Output into Sections (Texts, Data, @dots{})
@c the above section title is WAY too long. maybe cut the part between
diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c
index 0b32d5f..42c6973 100644
--- a/gcc/internal-fn.c
+++ b/gcc/internal-fn.c
@@ -2207,6 +2207,53 @@ expand_ATOMIC_COMPARE_EXCHANGE (internal_fn, gcall *call)
expand_ifn_atomic_compare_exchange (call);
}
+/* Expand DIVMOD() using:
In general, we do not use () when referring to objects in comments.
+ a) optab handler for udivmod/sdivmod if it is available.
+ b) If optab_handler doesn't exist, generate call to
+target-specific divmod libfunc. */
+
+static void
+expand_DIVMOD (internal_fn, gcall *call_stmt)
In general, don't use upper case for function names. Lower case please.
But I believe we should be pushing all the expansion stuff into the
gimple->rtl expansion code, so I haven't looked at this closely yet on
the expectation it'll likely move.
diff

On 19 October 2016 at 03:03, Jeff Law wrote:
> On 10/17/2016 11:23 PM, Prathamesh Kulkarni wrote:
>>
>> The divmod transform isn't enabled if target supports hardware div in
>> the same or wider mode even if divmod libfunc is available for the
>> given mode.
>
> Good. That seems like the right thing to do.
>
>> Thanks. I had erroneously assumed __udivimoddi4() was available for
>> all targets because it was defined in libgcc2.c and generated call to
>> it as "last resort" for unsigned DImode case if target didn't support
>> hardware div and divmod insn and didn't have target-specific divmod
>> libfunc for DImode. The reason why it generated undefined reference
>> on aarch64-linux-gnu was because I didn't properly check in the patch
>> if target supported hardware div, and ended up generating call to
>> __udivmoddi4() even though aarch64 has hardware div. I rectified
>> that before posting the patch.
>
> Understood. From a design standpoint, it seems to me like the path where we
> emit a call to udivmod without knowing if its supported by libgcc is broken.
> But if I understand correctly, that's not affected by your changes -- it's
> simply a historical poor decision.
Err no, that poor decision was entirely mine -;) I had wrongly
assumed __udivmoddi4 to be always available
and got surprised when it gave undefined reference error on aarch64
and hence brought it up for discussion.
I removed those parts of the patch that generated call to
__udivmoddi4() before posting the patch upstream.
>
>>>
>>> I don't even think we have a way of knowing in the compiler if the
>>> target has enabled divmod support in libgcc.
>>
>> Well the arm and c6x backends register target-specific divmod libfunc
>> via set_optab_libfunc(). I suppose that's sufficient to know if
>> target has divmod enabled in libgcc ?
>
> It's probably a pretty reasonable assumption that if the target has
> registered a libfunc, the the libfunc ought to be available.
>
>>>
>>> ISTM that for now we have to limit to cases where we have a divmod
>>> insn/libcall defined.
>>
>> Indeed. The transform is enabled only if the target has hardware
>> divmod insn or divmod libfunc (in the libfunc case, no hardware div
>> insn). Please see divmod_candidate_p() in the patch for cases when
>> the transform gets enabled.
>
> Great. Thanks. Hoping to make some progress on the actual patch in the
> next couple days.
Thanks!
Regards,
Prathamesh
>
> jeff

On 10/17/2016 11:23 PM, Prathamesh Kulkarni wrote:
The divmod transform isn't enabled if target supports hardware div in
the same or wider mode even if divmod libfunc is available for the
given mode.
Good. That seems like the right thing to do.
Thanks. I had erroneously assumed __udivimoddi4() was available for
all targets because it was defined in libgcc2.c and generated call to
it as "last resort" for unsigned DImode case if target didn't support
hardware div and divmod insn and didn't have target-specific divmod
libfunc for DImode. The reason why it generated undefined reference
on aarch64-linux-gnu was because I didn't properly check in the patch
if target supported hardware div, and ended up generating call to
__udivmoddi4() even though aarch64 has hardware div. I rectified
that before posting the patch.
Understood. From a design standpoint, it seems to me like the path
where we emit a call to udivmod without knowing if its supported by
libgcc is broken. But if I understand correctly, that's not affected by
your changes -- it's simply a historical poor decision.
I don't even think we have a way of knowing in the compiler if the
target has enabled divmod support in libgcc.
Well the arm and c6x backends register target-specific divmod libfunc
via set_optab_libfunc(). I suppose that's sufficient to know if
target has divmod enabled in libgcc ?
It's probably a pretty reasonable assumption that if the target has
registered a libfunc, the the libfunc ought to be available.
ISTM that for now we have to limit to cases where we have a divmod
insn/libcall defined.
Indeed. The transform is enabled only if the target has hardware
divmod insn or divmod libfunc (in the libfunc case, no hardware div
insn). Please see divmod_candidate_p() in the patch for cases when
the transform gets enabled.
Great. Thanks. Hoping to make some progress on the actual patch in the
next couple days.
jeff

On 10/18/2016 02:25 AM, Richard Biener wrote:
I don't even think we have a way of knowing in the compiler if the target
has enabled divmod support in libgcc.
Yeah, that's what bothers me with the current optab libfunc query
setup -- it isn't reliable.
I wonder if we ought to just have them all available in libgcc. If
they're one-function-per-file in the .a, then we're not going to get
significant codesize bloat in the resulting executables.
jeff

On 18 October 2016 at 13:55, Richard Biener wrote:
> On Tue, 18 Oct 2016, Prathamesh Kulkarni wrote:
>
>> On 18 October 2016 at 02:46, Jeff Law wrote:
>> > On 10/15/2016 11:59 PM, Prathamesh Kulkarni wrote:
>> >>
>> >> This patch is mostly the same as previous one, except it drops
>> >> targeting __udivmoddi4() because it gave undefined reference link
>> >> error for calling __udivmoddi4() on aarch64-linux-gnu. It appears
>> >> aarch64 has hardware insn for DImode div, so __udivmoddi4() isn't
>> >> needed for the target (it was a bug in my patch that called
>> >> __udivmoddi4() even though aarch64 supported hardware div).
>> >
>> > This touches on the one high level question I had. Namely what is the code
>> > generation strategy if the hardware has a div, but not divmod.
>> The divmod transform isn't enabled if target supports hardware div in the
>> same
>> or wider mode even if divmod libfunc is available for the given mode.
>> >
>> > ISTM in that case I think we want to use the div instruction and synthesize
>> > mod from that result rather than relying on a software divmod. So it looks
>> > like you ought to be doing the right thing for that case now based on your
>> > comment above.
>> >>
>> >>
>> >> However this makes me wonder if it's guaranteed that __udivmoddi4()
>> >> will be available for a target if it doesn't have hardware div and
>> >> divmod insn and doesn't have target-specific libfunc for DImode
>> >> divmod ? To be conservative, the attached patch doesn't generate call
>> >> to __udivmoddi4.
>> >
>> > I don't think that's a safe assumption. Addition of the divmod routines
>> > into libgcc is controlled by the target and have to be explicitly added
>> > AFAICT.
>> >
>> > So on a target like the fr30 which has no div or mod insn and doesn't
>> > define
>> > the right bits in libgcc, there is no divmod libcall available. (On these
>> > targets there's a div libcall and a mod libcall, but not a combined one).
>> Thanks. I had erroneously assumed __udivimoddi4() was available for all
>> targets
>> because it was defined in libgcc2.c and generated call to it as "last
>> resort" for unsigned
>> DImode case if target didn't support hardware div and divmod insn and
>> didn't have
>> target-specific divmod libfunc for DImode.
>> The reason why it generated undefined reference on aarch64-linux-gnu
>> was because I
>> didn't properly check in the patch if target supported hardware div,
>> and ended up generating call to
>> __udivmoddi4() even though aarch64 has hardware div. I rectified that
>> before posting the
>> patch.
>> >
>> > I don't even think we have a way of knowing in the compiler if the target
>> > has enabled divmod support in libgcc.
>
> Yeah, that's what bothers me with the current optab libfunc query
> setup -- it isn't reliable.
>
>> Well the arm and c6x backends register target-specific divmod libfunc via
>> set_optab_libfunc(). I suppose that's sufficient to know if target has
>> divmod enabled
>> in libgcc ?
>> >
>> > ISTM that for now we have to limit to cases where we have a divmod
>> > insn/libcall defined.
>> Indeed. The transform is enabled only if the target has hardware divmod insn
>> or divmod libfunc (in the libfunc case, no hardware div insn).
>> Please see divmod_candidate_p() in the patch for cases when the
>> transform gets enabled.
>
> But after your patch the divmod libfunc is never available?
After the patch, the divmod libfunc is not available if the target
doesn't explicitly set it.
So by default optab_libfunc ([us]divmod_optab, mode) would return NULL
instead of bogus libfunc
constructed with gen_int_libfunc, but if the target has registered it
with set_optab_libfunc, then
the target-specific libfunc name would be returned.
Thanks,
Prathamesh
>
> Richard.
>
>> Thanks,
>> Prathamesh
>> >
>> > jeff
>> >
>>
>>
>
> --
> Richard Biener
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
> 21284 (AG Nuernberg)

On Tue, 18 Oct 2016, Prathamesh Kulkarni wrote:
> On 18 October 2016 at 02:46, Jeff Law wrote:
> > On 10/15/2016 11:59 PM, Prathamesh Kulkarni wrote:
> >>
> >> This patch is mostly the same as previous one, except it drops
> >> targeting __udivmoddi4() because it gave undefined reference link
> >> error for calling __udivmoddi4() on aarch64-linux-gnu. It appears
> >> aarch64 has hardware insn for DImode div, so __udivmoddi4() isn't
> >> needed for the target (it was a bug in my patch that called
> >> __udivmoddi4() even though aarch64 supported hardware div).
> >
> > This touches on the one high level question I had. Namely what is the code
> > generation strategy if the hardware has a div, but not divmod.
> The divmod transform isn't enabled if target supports hardware div in the same
> or wider mode even if divmod libfunc is available for the given mode.
> >
> > ISTM in that case I think we want to use the div instruction and synthesize
> > mod from that result rather than relying on a software divmod. So it looks
> > like you ought to be doing the right thing for that case now based on your
> > comment above.
> >>
> >>
> >> However this makes me wonder if it's guaranteed that __udivmoddi4()
> >> will be available for a target if it doesn't have hardware div and
> >> divmod insn and doesn't have target-specific libfunc for DImode
> >> divmod ? To be conservative, the attached patch doesn't generate call
> >> to __udivmoddi4.
> >
> > I don't think that's a safe assumption. Addition of the divmod routines
> > into libgcc is controlled by the target and have to be explicitly added
> > AFAICT.
> >
> > So on a target like the fr30 which has no div or mod insn and doesn't define
> > the right bits in libgcc, there is no divmod libcall available. (On these
> > targets there's a div libcall and a mod libcall, but not a combined one).
> Thanks. I had erroneously assumed __udivimoddi4() was available for all
> targets
> because it was defined in libgcc2.c and generated call to it as "last
> resort" for unsigned
> DImode case if target didn't support hardware div and divmod insn and
> didn't have
> target-specific divmod libfunc for DImode.
> The reason why it generated undefined reference on aarch64-linux-gnu
> was because I
> didn't properly check in the patch if target supported hardware div,
> and ended up generating call to
> __udivmoddi4() even though aarch64 has hardware div. I rectified that
> before posting the
> patch.
> >
> > I don't even think we have a way of knowing in the compiler if the target
> > has enabled divmod support in libgcc.
Yeah, that's what bothers me with the current optab libfunc query
setup -- it isn't reliable.
> Well the arm and c6x backends register target-specific divmod libfunc via
> set_optab_libfunc(). I suppose that's sufficient to know if target has
> divmod enabled
> in libgcc ?
> >
> > ISTM that for now we have to limit to cases where we have a divmod
> > insn/libcall defined.
> Indeed. The transform is enabled only if the target has hardware divmod insn
> or divmod libfunc (in the libfunc case, no hardware div insn).
> Please see divmod_candidate_p() in the patch for cases when the
> transform gets enabled.
But after your patch the divmod libfunc is never available?
Richard.
> Thanks,
> Prathamesh
> >
> > jeff
> >
>
>
--
Richard Biener
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)

On 18 October 2016 at 02:46, Jeff Law wrote:
> On 10/15/2016 11:59 PM, Prathamesh Kulkarni wrote:
>>
>> This patch is mostly the same as previous one, except it drops
>> targeting __udivmoddi4() because it gave undefined reference link
>> error for calling __udivmoddi4() on aarch64-linux-gnu. It appears
>> aarch64 has hardware insn for DImode div, so __udivmoddi4() isn't
>> needed for the target (it was a bug in my patch that called
>> __udivmoddi4() even though aarch64 supported hardware div).
>
> This touches on the one high level question I had. Namely what is the code
> generation strategy if the hardware has a div, but not divmod.
The divmod transform isn't enabled if target supports hardware div in the same
or wider mode even if divmod libfunc is available for the given mode.
>
> ISTM in that case I think we want to use the div instruction and synthesize
> mod from that result rather than relying on a software divmod. So it looks
> like you ought to be doing the right thing for that case now based on your
> comment above.
>>
>>
>> However this makes me wonder if it's guaranteed that __udivmoddi4()
>> will be available for a target if it doesn't have hardware div and
>> divmod insn and doesn't have target-specific libfunc for DImode
>> divmod ? To be conservative, the attached patch doesn't generate call
>> to __udivmoddi4.
>
> I don't think that's a safe assumption. Addition of the divmod routines
> into libgcc is controlled by the target and have to be explicitly added
> AFAICT.
>
> So on a target like the fr30 which has no div or mod insn and doesn't define
> the right bits in libgcc, there is no divmod libcall available. (On these
> targets there's a div libcall and a mod libcall, but not a combined one).
Thanks. I had erroneously assumed __udivimoddi4() was available for all targets
because it was defined in libgcc2.c and generated call to it as "last
resort" for unsigned
DImode case if target didn't support hardware div and divmod insn and
didn't have
target-specific divmod libfunc for DImode.
The reason why it generated undefined reference on aarch64-linux-gnu
was because I
didn't properly check in the patch if target supported hardware div,
and ended up generating call to
__udivmoddi4() even though aarch64 has hardware div. I rectified that
before posting the
patch.
>
> I don't even think we have a way of knowing in the compiler if the target
> has enabled divmod support in libgcc.
Well the arm and c6x backends register target-specific divmod libfunc via
set_optab_libfunc(). I suppose that's sufficient to know if target has
divmod enabled
in libgcc ?
>
> ISTM that for now we have to limit to cases where we have a divmod
> insn/libcall defined.
Indeed. The transform is enabled only if the target has hardware divmod insn
or divmod libfunc (in the libfunc case, no hardware div insn).
Please see divmod_candidate_p() in the patch for cases when the
transform gets enabled.
Thanks,
Prathamesh
>
> jeff
>

On 10/15/2016 11:59 PM, Prathamesh Kulkarni wrote:
This patch is mostly the same as previous one, except it drops
targeting __udivmoddi4() because it gave undefined reference link
error for calling __udivmoddi4() on aarch64-linux-gnu. It appears
aarch64 has hardware insn for DImode div, so __udivmoddi4() isn't
needed for the target (it was a bug in my patch that called
__udivmoddi4() even though aarch64 supported hardware div).
This touches on the one high level question I had. Namely what is the
code generation strategy if the hardware has a div, but not divmod.
ISTM in that case I think we want to use the div instruction and
synthesize mod from that result rather than relying on a software
divmod. So it looks like you ought to be doing the right thing for that
case now based on your comment above.
However this makes me wonder if it's guaranteed that __udivmoddi4()
will be available for a target if it doesn't have hardware div and
divmod insn and doesn't have target-specific libfunc for DImode
divmod ? To be conservative, the attached patch doesn't generate call
to __udivmoddi4.
I don't think that's a safe assumption. Addition of the divmod routines
into libgcc is controlled by the target and have to be explicitly added
AFAICT.
So on a target like the fr30 which has no div or mod insn and doesn't
define the right bits in libgcc, there is no divmod libcall available.
(On these targets there's a div libcall and a mod libcall, but not a
combined one).
I don't even think we have a way of knowing in the compiler if the
target has enabled divmod support in libgcc.
ISTM that for now we have to limit to cases where we have a divmod
insn/libcall defined.
jeff

On 16 October 2016 at 11:29, Prathamesh Kulkarni
wrote:
> Hi,
> After approval from Bernd Schmidt, I committed the patch to remove
> optab functions for
> sdivmod_optab and udivmod_optab in optabs.def, which removes the block
> for divmod patch.
>
> This patch is mostly the same as previous one, except it drops
> targeting __udivmoddi4() because
> it gave undefined reference link error for calling __udivmoddi4() on
> aarch64-linux-gnu.
> It appears aarch64 has hardware insn for DImode div, so __udivmoddi4()
> isn't needed for the target
> (it was a bug in my patch that called __udivmoddi4() even though
> aarch64 supported hardware div).
>
> However this makes me wonder if it's guaranteed that __udivmoddi4()
> will be available for a target if it doesn't have hardware div and
> divmod insn and doesn't have target-specific libfunc for
> DImode divmod ? To be conservative, the attached patch doesn't
> generate call to __udivmoddi4.
>
> Passes bootstrap+test on x86_64-unknown-linux.
> Cross-tested on arm*-*-*, aarch64*-*-*.
> Verified that there are no regressions with SPEC2006 on
I mean no regressions from correctness perspective, I didn't benchmark
the patch.
> x86_64-unknown-linux-gnu.
> OK to commit ?
>
> Thanks,
> Prathamesh