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.