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.
Richard.
>
> 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 (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
> >>
> >>
> >
> > --
> > Richard Biener <rguent...@suse.de>
> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
> > 21284 (AG Nuernberg)
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
