On Fri, May 18, 2018 at 4:37 AM Kugan Vivekanandarajah <
kugan.vivekanandara...@linaro.org> wrote:
> 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.
Well, all existing ABS_EXPR generations are obviously fine. Then there
are transforms that are not possible w/o ABSU_EXPR like the narrowing
you performed here. This is becasue for ABS_EXPR you can't simply avoid
the undefined
behavior by performing the operation on unsigned integers... (that's similar
to signed integer division of -INT_MIN / -1 -- the result is representable
in
unsigned but not in signed).
> >
> > (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.
