RFC [1/2] divmod transform

On 23 May 2016 at 17:35, Richard Biener <richard.guenther@gmail.com> wrote:
> 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.
Well I would prefer adding the hook because that's more easier -;)
Would it be ok for now to go with the 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?
Ah indeed, the check for DImode is unnecessary.
However I suppose the check for unsignedp should be there,
since we want to generate call to __udivmoddi4 only if operand is unsigned ?
>
> 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).
Thanks for pointing out. So if a target does not support divmod
libfunc for a mode
but for a wider mode, then we could zero-extend operands to the wider-mode,
perform divmod on the wider-mode, and then cast result back to the
original mode.
I haven't done that in this patch, would it be OK to do that as a follow up ?
>
> +  /* 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).
I added TYPE_OVERFLOW_TRAPS (type) based on your suggestion in:
https://www.mail-archive.com/gcc@gcc.gnu.org/msg78534.html
"When looking at TRUNC_DIV_EXPR you should also exclude
the case where TYPE_OVERFLOW_TRAPS (type) as that should
expand using the [su]divv optabs (no trapping overflow
divmod optab exists)."
>
> +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.
Done changes in this version. I am gathering mod uses same time as div uses,
so this imposes a constraint that mod dominates mod. I am not sure if
that's desirable.

Thanks,
Prathamesh
>
> Thanks,
> Richard.
>
>> Thanks,
>> Prathamesh

RFC [1/2] divmod transform

Commit Message

Comments

Patch