On 26 October 2016 at 16:17, Richard Biener <rguent...@suse.de> wrote:
> On Wed, 26 Oct 2016, Prathamesh Kulkarni wrote:
>
>> On 25 October 2016 at 18:47, Richard Biener <rguent...@suse.de> wrote:
>> > On Tue, 25 Oct 2016, Prathamesh Kulkarni wrote:
>> >
>> >> On 25 October 2016 at 16:17, Richard Biener <rguent...@suse.de> wrote:
>> >> > On Tue, 25 Oct 2016, Prathamesh Kulkarni wrote:
>> >> >
>> >> >> On 25 October 2016 at 13:43, Richard Biener
>> >> >> <richard.guent...@gmail.com> wrote:
>> >> >> > On Sun, Oct 16, 2016 at 7:59 AM, Prathamesh Kulkarni
>> >> >> > <prathamesh.kulka...@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.
Committed as r241660.
Thanks a lot!
Regards,
Prathamesh
>
> Thanks,
> Richard.
