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 (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. @@ -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? @@ -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. 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. 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. 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.