Hi Richard,
Thanks for the review. I am revising the patch based on Andrew's comments too.
On 17 May 2018 at 20:36, Richard Biener <richard.guent...@gmail.com> wrote:
> On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pins...@gmail.com> wrote:
>
>> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah
>> <kugan.vivekanandara...@linaro.org> wrote:
>> > As mentioned in the PR, I am trying to add ABSU_EXPR to fix this
>> > issue. In the attached patch, in fold_cond_expr_with_comparison I am
>> > generating ABSU_EXPR for these cases. As I understand, absu_expr is
>> > well defined in RTL. So, the issue is generating absu_expr and
>> > transferring to RTL in the correct way. I am not sure I am not doing
>> > all that is needed. I will clean up and add more test-cases based on
>> > the feedback.
>
>
>> diff --git a/gcc/optabs-tree.c b/gcc/optabs-tree.c
>> index 71e172c..2b812e5 100644
>> --- a/gcc/optabs-tree.c
>> +++ b/gcc/optabs-tree.c
>> @@ -235,6 +235,7 @@ optab_for_tree_code (enum tree_code code, const_tree
> type,
>> return trapv ? negv_optab : neg_optab;
>
>> case ABS_EXPR:
>> + case ABSU_EXPR:
>> return trapv ? absv_optab : abs_optab;
>
>
>> This part is not correct, it should something like this:
>
>> case ABS_EXPR:
>> return trapv ? absv_optab : abs_optab;
>> + case ABSU_EXPR:
>> + return abs_optab ;
>
>> Because ABSU is not undefined at the TYPE_MAX.
>
> Also
>
> /* Unsigned abs is simply the operand. Testing here means we don't
> risk generating incorrect code below. */
> - if (TYPE_UNSIGNED (type))
> + if (TYPE_UNSIGNED (type)
> + && (code != ABSU_EXPR))
> return op0;
>
> is wrong. ABSU of an unsigned number is still just that number.
>
> The change to fold_cond_expr_with_comparison looks odd to me
> (premature optimization). It should be done separately - it seems
> you are doing
FE seems to be using this to generate ABS_EXPR from
c_fully_fold_internal to fold_build3_loc and so on. I changed this to
generate ABSU_EXPR for the case in the testcase. So the question
should be, in what cases do we need ABS_EXPR and in what cases do we
need ABSU_EXPR. It is not very clear to me.
>
> (simplify (abs (convert @0)) (convert (absu @0)))
>
> here.
>
> You touch one other place in fold-const.c but there seem to be many
> more that need ABSU_EXPR handling (you touched the one needed
> for correctness) - esp. you should at least handle constant folding
> in const_unop and the nonnegative predicate.
OK.
>
> @@ -3167,6 +3167,9 @@ verify_expr (tree *tp, int *walk_subtrees, void *data
> ATTRIBUTE_UNUSED)
> CHECK_OP (0, "invalid operand to unary operator");
> break;
>
> + case ABSU_EXPR:
> + break;
> +
> case REALPART_EXPR:
> case IMAGPART_EXPR:
>
> verify_expr is no more. Did you test this recently against trunk?
This patch is against slightly older trunk. I will rebase it.
>
> @@ -3937,6 +3940,9 @@ verify_gimple_assign_unary (gassign *stmt)
> case PAREN_EXPR:
> case CONJ_EXPR:
> break;
> + case ABSU_EXPR:
> + /* FIXME. */
> + return false;
>
> no - please not! Please add verification here - ABSU should be only
> called on INTEGRAL, vector or complex INTEGRAL types and the
> type of the LHS should be always the unsigned variant of the
> argument type.
OK.
>
> if (is_gimple_val (cond_expr))
> return cond_expr;
>
> - if (TREE_CODE (cond_expr) == ABS_EXPR)
> + if (TREE_CODE (cond_expr) == ABS_EXPR
> + || TREE_CODE (cond_expr) == ABSU_EXPR)
> {
> rhs1 = TREE_OPERAND (cond_expr, 1);
> STRIP_USELESS_TYPE_CONVERSION (rhs1);
>
> err, but the next line just builds a ABS_EXPR ...
>
> How did you identify spots that need adjustment? I would expect that
> once folding generates ABSU_EXPR that you need to adjust frontends
> (C++ constexpr handling for example). Also I miss adjustments
> to gimple-pretty-print.c and the GIMPLE FE parser.
I will add this.
>
> recursively grepping throughout the whole gcc/ tree doesn't reveal too many
> cases of ABS_EXPR so I think it's reasonable to audit all of them.
>
> I also miss some trivial absu simplifications in match.pd. There are not
> a lot of abs cases but similar ones would be good to have initially.
I will add them in the next version.
Thanks,
Kugan
>
> Thanks for tackling this!
> Richard.
>
>> Thanks,
>> Andrew
>
>> >
>> > Thanks,
>> > Kugan
>> >
>> >
>> > gcc/ChangeLog:
>> >
>> > 2018-05-13 Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org>
>> >
>> > * expr.c (expand_expr_real_2): Handle ABSU_EXPR.
>> > * fold-const.c (fold_cond_expr_with_comparison): Generate ABSU_EXPR
>> > (fold_unary_loc): Handle ABSU_EXPR.
>> > * optabs-tree.c (optab_for_tree_code): Likewise.
>> > * tree-cfg.c (verify_expr): Likewise.
>> > (verify_gimple_assign_unary): Likewise.
>> > * tree-if-conv.c (fold_build_cond_expr): Likewise.
>> > * tree-inline.c (estimate_operator_cost): Likewise.
>> > * tree-pretty-print.c (dump_generic_node): Likewise.
>> > * tree.def (ABSU_EXPR): New.
>> >
>> > gcc/testsuite/ChangeLog:
>> >
>> > 2018-05-13 Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org>
>> >
>> > * gcc.dg/absu.c: New test.
