RFC [1/3] divmod transform v2

Richard Biener richard.guenther@gmail.com
Tue Oct 25 08:13:00 GMT 2016


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.

+      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



More information about the Gcc-patches mailing list