On Mon, Jun 4, 2018 at 10:18 AM Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org> wrote: > > Hi Richard, > > Thanks for the review. > > On 1 June 2018 at 22:20, Richard Biener <richard.guent...@gmail.com> wrote: > > On Fri, Jun 1, 2018 at 4:12 AM Kugan Vivekanandarajah > > <kugan.vivekanandara...@linaro.org> wrote: > >> > >> Hi Richard, > >> > >> This is the revised patch based on the review and the discussion in > >> https://gcc.gnu.org/ml/gcc/2018-05/msg00179.html. > >> > >> In summary: > >> - I skipped (element_precision (type) < element_precision (TREE_TYPE > >> (@0))) in the match.pd pattern as this would prevent transformation > >> for the case in PR. > >> that is, I am interested in is something like: > >> char t = (char) ABS_EXPR <(int) x> > >> and I want to generate > >> char t = (char) ABSU_EXPR <x> > >> > >> - I also haven't added all the necessary match.pd changes for > >> ABSU_EXPR. I have a patch for that but will submit separately based on > >> this reveiw. > >> > >> - I also tried to add ABSU_EXPRsupport in the places as necessary by > >> grepping for ABS_EXPR. > >> > >> - I also had to add special casing in vectorizer for ABSU_EXP as its > >> result is unsigned type. > >> > >> Is this OK. Patch bootstraps and the regression test is ongoing. > > > > The c/c-typeck.c:build_unary_op change looks unnecessary - the > > C FE should never generate this directly (the c-common one might > > be triggered by early folding I guess). > > The Gimple FE testcase is running into this.
Ah, OK then. > > > > @@ -1761,6 +1762,9 @@ const_unop (enum tree_code code, tree type, tree arg0) > > if (TREE_CODE (arg0) == INTEGER_CST || TREE_CODE (arg0) == REAL_CST) > > return fold_abs_const (arg0, type); > > break; > > + case ABSU_EXPR: > > + return fold_convert (type, fold_abs_const (arg0, > > + signed_type_for (type))); > > > > case CONJ_EXPR: > > > > I think this will get you bogus TREE_OVERFLOW flags set on ABSU (-INT_MIN). > > > > I think you want to change fold_abs_const to properly deal with arg0 being > > signed and type unsigned. That is, sth like > > > > diff --git a/gcc/fold-const.c b/gcc/fold-const.c > > index 6f80f1b1d69..f60f9c77e91 100644 > > --- a/gcc/fold-const.c > > +++ b/gcc/fold-const.c > > @@ -13843,18 +13843,19 @@ fold_abs_const (tree arg0, tree type) > > { > > /* If the value is unsigned or non-negative, then the absolute > > value > > is the same as the ordinary value. */ > > - if (!wi::neg_p (wi::to_wide (arg0), TYPE_SIGN (type))) > > - t = arg0; > > + wide_int val = wi::to_wide (arg0); > > + bool overflow = false; > > + if (!wi::neg_p (val, TYPE_SIGN (TREE_TYPE (arg0)))) > > + ; > > > > /* If the value is negative, then the absolute value is > > its negation. */ > > else > > - { > > - bool overflow; > > - wide_int val = wi::neg (wi::to_wide (arg0), &overflow); > > - t = force_fit_type (type, val, -1, > > - overflow | TREE_OVERFLOW (arg0)); > > - } > > + wide_int val = wi::neg (val, &overflow); > > + > > + /* Force to the destination type, set TREE_OVERFLOW for signed > > + TYPE only. */ > > + t = force_fit_type (type, val, 1, overflow | TREE_OVERFLOW (arg0)); > > } > > break; > > > > and then simply share the const_unop code with ABS_EXPR. > > Done. > > > diff --git a/gcc/match.pd b/gcc/match.pd > > index 14386da..7d7c132 100644 > > --- a/gcc/match.pd > > +++ b/gcc/match.pd > > @@ -102,6 +102,14 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > > (match (nop_convert @0) > > @0) > > > > +(simplify (abs (convert @0)) > > + (if (ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0)) > > + && !TYPE_UNSIGNED (TREE_TYPE (@0)) > > + && !TYPE_UNSIGNED (type)) > > + (with { tree utype = unsigned_type_for (TREE_TYPE (@0)); } > > + (convert (absu:utype @0))))) > > + > > + > > > > please put a comment before the pattern. I believe there's no > > need to check for !TYPE_UNSIGNED (type). Note this > > also converts abs ((char)int-var) to (char)absu(int-var) which > > doesn't make sense. The original issue you want to address > > here is the case where TYPE_PRECISION of @0 is less than > > the precision of type. That is, you want to remove language > > introduced integer promotion of @0 which only is possible > > with ABSU. So please do add such precision check > > (I simply suggested the bogus direction of the test). > > Done. > > > > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > > index 68f4fd3..9b62583 100644 > > --- a/gcc/tree-cfg.c > > +++ b/gcc/tree-cfg.c > > @@ -3685,6 +3685,12 @@ verify_gimple_assign_unary (gassign *stmt) > > case PAREN_EXPR: > > case CONJ_EXPR: > > break; > > + case ABSU_EXPR: > > + if (!TYPE_UNSIGNED (lhs_type) > > + || !ANY_INTEGRAL_TYPE_P (rhs1_type)) > > > > if (!ANY_INTEGRAL_TYPE_P (lhs_type) > > || !TYPE_UNSIGNED (lhs_type) > > || !ANY_INTEGRAL_TYPE_P (rhs1_type) > > || TYPE_UNSIGNED (rhs1_type) > > || element_precision (lhs_type) != element_precision (rhs1_type)) > > { > > error ("invalid types for ABSU_EXPR"); > > debug_generic_expr (lhs_type); > > debug_generic_expr (rhs1_type); > > return true; > > } > > ^^^ you forgot this one. > > + return true; > > + return false; > > + break; > > > > diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c > > index 30c6d9e..44b1399 100644 > > --- a/gcc/tree-eh.c > > +++ b/gcc/tree-eh.c > > @@ -2465,6 +2465,7 @@ operation_could_trap_helper_p (enum tree_code op, > > > > case NEGATE_EXPR: > > case ABS_EXPR: > > + case ABSU_EXPR: > > case CONJ_EXPR: > > /* These operations don't trap with floating point. */ > > if (honor_trapv) > > > > ABSU never traps. Please instead unconditionally return false. > Done. > > > > > diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c > > index 66c78de..b52d714 100644 > > --- a/gcc/tree-vect-stmts.c > > +++ b/gcc/tree-vect-stmts.c > > @@ -5995,7 +5995,11 @@ vectorizable_operation (gimple *stmt, > > gimple_stmt_iterator *gsi, > > "transform binary/unary operation.\n"); > > > > /* Handle def. */ > > - vec_dest = vect_create_destination_var (scalar_dest, vectype); > > + if (code == ABSU_EXPR) > > + vec_dest = vect_create_destination_var (scalar_dest, > > + unsigned_type_for (vectype)); > > + else > > + vec_dest = vect_create_destination_var (scalar_dest, vectype); > > > > /* POINTER_DIFF_EXPR has pointer arguments which are vectorized as > > vectors with unsigned elements, but the result is signed. So, we > > > > simply use vectype_out for creation of vec_dest. > Done. /* Handle def. */ - vec_dest = vect_create_destination_var (scalar_dest, vectype); + if (code == ABSU_EXPR) + vec_dest = vect_create_destination_var (scalar_dest, vectype_out); + else + vec_dest = vect_create_destination_var (scalar_dest, vectype); I meant _always_ vectype_out. Thus unconditionally vec_dest = vect_create_destination_var (scalar_dest, vectype_out); OK with those two changes. Thanks, Richard. > > > > diff --git a/gcc/tree.def b/gcc/tree.def > > index c660b2c..5fec781 100644 > > --- a/gcc/tree.def > > +++ b/gcc/tree.def > > @@ -760,6 +760,7 @@ DEFTREECODE (MAX_EXPR, "max_expr", tcc_binary, 2) > > An ABS_EXPR must have either an INTEGER_TYPE or a REAL_TYPE. The > > operand of the ABS_EXPR must have the same type. */ > > DEFTREECODE (ABS_EXPR, "abs_expr", tcc_unary, 1) > > +DEFTREECODE (ABSU_EXPR, "absu_expr", tcc_unary, 1) > > > > /* Shift operations for shift and rotate. > > Shift means logical shift if done on an > > > > You can clearly see that the comment before ABS_EXPR doesn't apply to > > ABSU_EXPR > > so please add an appropriate one. I suggest > > > > /* Represents the unsigned absolute value of the operand. > > An ABSU_EXPR must have unsigned INTEGER_TYPE. The operand of the > > ABSU_EXPR > > must have the corresponding signed type. */ > > Done. > > Here is the reviesed patch. Is this OK? > > Thanks, > Kugan > > > > > Otherwise looks OK. (I didn't explicitely check for missing ABSU_EXPR > > handling this time) > > > > Thanks, > > Richard. > > > > > >> Thanks, > >> Kugan > >> > >> > >> On 18 May 2018 at 12:36, 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. > >> > > >> > > >> >> > >> >> (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.