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