Re: RFC [1/3] divmod transform v2

[Richard Biener <rguenther at suse dot de>]

On Wed, 26 Oct 2016, Prathamesh Kulkarni wrote:

> On 25 October 2016 at 18:47, Richard Biener <rguenther@suse.de> wrote:
> > On Tue, 25 Oct 2016, Prathamesh Kulkarni wrote:
> >
> >> On 25 October 2016 at 16:17, Richard Biener <rguenther@suse.de> wrote:
> >> > On Tue, 25 Oct 2016, Prathamesh Kulkarni wrote:
> >> >
> >> >> On 25 October 2016 at 13:43, Richard Biener <richard.guenther@gmail.com> wrote:
> >> >> > On Sun, Oct 16, 2016 at 7:59 AM, Prathamesh Kulkarni
> >> >> > <prathamesh.kulkarni@linaro.org> 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 (&use_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 (&use_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) ?
> >
> > Ah, yes - didn't spot that.
> Hi,
> Is this version OK ?

Yes.

Thanks,
Richard.