RFC [1/2] divmod transform

[Richard Biener]

On Mon, May 23, 2016 at 10:58 AM, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
> Hi,
> I have updated my patch for divmod (attached), which was originally
> based on Kugan's patch.
> The patch transforms stmts with code TRUNC_DIV_EXPR and TRUNC_MOD_EXPR
> having same operands to divmod representation, so we can cse computation of mod.
>
> t1 = a TRUNC_DIV_EXPR b;
> t2 = a TRUNC_MOD_EXPR b
> is transformed to:
> complex_tmp = DIVMOD (a, b);
> t1 = REALPART_EXPR (complex_tmp);
> t2 = IMAGPART_EXPR (complex_tmp);
>
> * New hook divmod_expand_libfunc
> The rationale for introducing the hook is that different targets have
> incompatible calling conventions for divmod libfunc.
> Currently three ports define divmod libfunc: c6x, spu and arm.
> c6x and spu follow the convention of libgcc2.c:__udivmoddi4:
> return quotient and store remainder in argument passed as pointer,
> while the arm version takes two arguments and returns both
> quotient and remainder having mode double the size of the operand mode.
> The port should hence override the hook expand_divmod_libfunc
> to generate call to target-specific divmod.
> Ports should define this hook if:
> a) The port does not have divmod or div insn for the given mode.
> b) The port defines divmod libfunc for the given mode.
> The default hook default_expand_divmod_libfunc() generates call
> to libgcc2.c:__udivmoddi4 provided the operands are unsigned and
> are of DImode.
>
> Patch passes bootstrap+test on x86_64-unknown-linux-gnu and
> cross-tested on arm*-*-*.
> Bootstrap+test in progress on arm-linux-gnueabihf.
> Does this patch look OK ?

diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index 6b4601b..e4a021a 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1965,4 +1965,31 @@ default_optab_supported_p (int, machine_mode,
machine_mode, optimization_type)
   return true;
 }

+void
+default_expand_divmod_libfunc (bool unsignedp, machine_mode mode,
+                              rtx op0, rtx op1,
+                              rtx *quot_p, rtx *rem_p)

functions need a comment.

ISTR it was suggested that ARM change to libgcc2.c__udivmoddi4 style?  In that
case we could avoid the target hook.

+      /* If target overrides expand_divmod_libfunc hook
+        then perform divmod by generating call to the target-specifc divmod
libfunc.  */
+      if (targetm.expand_divmod_libfunc != default_expand_divmod_libfunc)
+       return true;
+
+      /* Fall back to using libgcc2.c:__udivmoddi4.  */
+      return (mode == DImode && unsignedp);

I don't understand this - we know optab_libfunc returns non-NULL for 'mode'
but still restrict this to DImode && unsigned?  Also if
targetm.expand_divmod_libfunc
is not the default we expect the target to handle all modes?

That said - I expected the above piece to be simply a 'return true;' ;)

Usually we use some can_expand_XXX helper in optabs.c to query if the target
supports a specific operation (for example SImode divmod would use DImode
divmod by means of widening operands - for the unsigned case of course).

+  /* Disable the transform if either is a constant, since
division-by-constant
+     may have specialized expansion.  */
+  if (TREE_CONSTANT (op1) || TREE_CONSTANT (op2))
+    return false;

please use CONSTANT_CLASS_P (op1) || CONSTANT_CLASS_P (op2)

+  if (TYPE_OVERFLOW_TRAPS (type))
+    return false;

why's that?  Generally please first test cheap things (trapping, constant-ness)
before checking expensive stuff (target_supports_divmod_p).

+static bool
+convert_to_divmod (gassign *stmt)
+{
+  if (!divmod_candidate_p (stmt))
+    return false;
+
+  tree op1 = gimple_assign_rhs1 (stmt);
+  tree op2 = gimple_assign_rhs2 (stmt);
+
+  vec<gimple *> stmts = vNULL;

use an auto_vec <gimple *> - you currently leak it in at least one place.

+      if (maybe_clean_or_replace_eh_stmt (use_stmt, use_stmt))
+       cfg_changed = true;

note that this suggests you should check whether any of the stmts may throw
internally as you don't update / transfer EH info correctly.  So for 'stmt' and
all 'use_stmt' check stmt_can_throw_internal and if so do not add it to
the list of stmts to modify.

Btw, I think you should not add 'stmt' immediately but when iterating over
all uses also gather uses in TRUNC_MOD_EXPR.

Otherwise looks ok.

Thanks,
Richard.

> Thanks,
> Prathamesh