[RFC][PR64946] "abs" vectorization fails for char/short types
Richard Biener
richard.guenther@gmail.com
Fri Jun 1 12:21:00 GMT 2018
On Fri, Jun 1, 2018 at 4:12 AM Kugan Vivekanandarajah
<kugan.vivekanandarajah@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).
@@ -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.
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).
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;
}
+ 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.
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.
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. */
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.vivekanandarajah@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.guenther@gmail.com> wrote:
> >> On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pinskia@gmail.com> wrote:
> >>
> >>> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah
> >>> <kugan.vivekanandarajah@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.vivekanandarajah@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.vivekanandarajah@linaro.org>
> >>> >
> >>> > * gcc.dg/absu.c: New test.
More information about the Gcc-patches
mailing list