RFC [1/3] divmod transform v2

Prathamesh Kulkarni prathamesh.kulkarni@linaro.org
Tue Oct 25 12:14:00 GMT 2016


On 25 October 2016 at 16:17, Richard Biener <rguenther@suse.de> wrote:
> On Tue, 25 Oct 2016, Prathamesh Kulkarni wrote:
>
>> On 25 October 2016 at 13:43, Richard Biener <richard.guenther@gmail.com> wrote:
>> > 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.
>> 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.
Sure, I will move that up and do the other suggested changes.

I was wondering if this condition in 2nd loop is too restrictive ?
if (!dominated_by_p (CDI_DOMINATORS, gimple_bb (use_stmt), top_bb))
  {
    end_imm_use_stmt_traverse (&use_iter);
    return false;
  }

Should we rather "continue" in this case by not adding use_stmt to
stmts vector rather than dropping
the transform all-together if gimple_bb (use_stmt) is not dominated by
gimple_bb (top_stmt) ?

For instance if we have a test-case like:

if (cond)
{
  t1 = x / y;
  t2 = x % y;
}
else
  t3 = x % y;

and suppose stmt is "t2 = x % y", we would set top_stmt to "t1 = x / y";
In this case we would still want to do divmod transform in THEN block
even though "t3 = x % y" is not dominated by top_stmt ?

if (cond)
{
  divmod_tmp = DIVMOD (x, y);
  t1 = REALPART_EXPR (divmod_tmp);
  t2 = IMAGPART_EXPR (divmod_tmp);
}
else
  t3 = x % y;

We will always ensure that all the trunc_div, trunc_mod statements in
stmts vector will be dominated by top_stmt,
but I suppose they need not constitute all the trunc_div, trunc_mod
statements in the function.

Thanks,
Prathamesh
>
> 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 <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)



More information about the Gcc-patches mailing list