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.
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)
