On 25 October 2016 at 13:43, Richard Biener <richard.guent...@gmail.com> wrote:
> 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.
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 ?
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
