| Message ID | CAELXzTPFP5QkdDRscDGTaEPwTLe_ZzrGkuSimhm+DZDfK6dw_w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,PR64946] "abs" vectorization fails for char/short types | expand |
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. 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.
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 (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.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.
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.
On Fri, May 18, 2018 at 4:37 AM 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. 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.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.
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. 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. From 6675f0fe71b1de2c2def5c5d2bd150ae1e707b5c Mon Sep 17 00:00:00 2001 From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> Date: Tue, 29 May 2018 10:26:23 +1000 Subject: [PATCH] absu v2 Change-Id: Id34948b4d585fbcdcb8ff6eb3728fd3cc6c41bfa --- gcc/c-family/c-common.c | 1 + gcc/c/c-typeck.c | 10 ++++++++++ gcc/c/gimple-parser.c | 9 ++++++++- gcc/cfgexpand.c | 1 + gcc/config/i386/i386.c | 1 + gcc/cp/constexpr.c | 1 + gcc/cp/cp-gimplify.c | 1 + gcc/dojump.c | 1 + gcc/expr.c | 3 ++- gcc/fold-const.c | 6 +++++- gcc/gimple-pretty-print.c | 7 +++++-- gcc/gimple-ssa-backprop.c | 2 ++ gcc/match.pd | 8 ++++++++ gcc/optabs-tree.c | 2 ++ gcc/testsuite/gcc.dg/absu.c | 41 ++++++++++++++++++++++++++++++++++++++ gcc/testsuite/gcc.dg/gimplefe-29.c | 11 ++++++++++ gcc/tree-cfg.c | 6 ++++++ gcc/tree-eh.c | 1 + gcc/tree-inline.c | 1 + gcc/tree-pretty-print.c | 6 ++++++ gcc/tree-vect-patterns.c | 3 ++- gcc/tree-vect-stmts.c | 6 +++++- gcc/tree.def | 1 + 23 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/absu.c create mode 100644 gcc/testsuite/gcc.dg/gimplefe-29.c diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 859eeb4..0e8efb5 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -3312,6 +3312,7 @@ c_common_truthvalue_conversion (location_t location, tree expr) case NEGATE_EXPR: case ABS_EXPR: + case ABSU_EXPR: case FLOAT_EXPR: case EXCESS_PRECISION_EXPR: /* These don't change whether an object is nonzero or zero. */ diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 45a4529..5bb6804 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -4314,6 +4314,16 @@ build_unary_op (location_t location, enum tree_code code, tree xarg, arg = default_conversion (arg); break; + case ABSU_EXPR: + if (!(typecode == INTEGER_TYPE)) + { + error_at (location, "wrong type argument to absu"); + return error_mark_node; + } + else if (!noconvert) + arg = default_conversion (arg); + break; + case CONJ_EXPR: /* Conjugating a real value is a no-op, but allow it anyway. */ if (!(typecode == INTEGER_TYPE || typecode == REAL_TYPE diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index 8f1c442..1be5d14 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -328,7 +328,8 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) case CPP_NAME: { tree id = c_parser_peek_token (parser)->value; - if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0) + if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0 + || strcmp (IDENTIFIER_POINTER (id), "__ABSU") == 0) goto build_unary_expr; break; } @@ -638,6 +639,12 @@ c_parser_gimple_unary_expression (c_parser *parser) op = c_parser_gimple_postfix_expression (parser); return parser_build_unary_op (op_loc, ABS_EXPR, op); } + else if (strcmp (IDENTIFIER_POINTER (id), "__ABSU") == 0) + { + c_parser_consume_token (parser); + op = c_parser_gimple_postfix_expression (parser); + return parser_build_unary_op (op_loc, ABSU_EXPR, op); + } else return c_parser_gimple_postfix_expression (parser); } diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 5c323be..d06224d 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -4620,6 +4620,7 @@ expand_debug_expr (tree exp) } case ABS_EXPR: + case ABSU_EXPR: return simplify_gen_unary (ABS, mode, op0, mode); case NEGATE_EXPR: diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 637c105..f5a4856 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -51232,6 +51232,7 @@ ix86_add_stmt_cost (void *data, int count, enum vect_cost_for_stmt kind, case BIT_IOR_EXPR: case ABS_EXPR: + case ABSU_EXPR: case MIN_EXPR: case MAX_EXPR: case BIT_XOR_EXPR: diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index a099408..30ea091 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -5759,6 +5759,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now, case FLOAT_EXPR: case NEGATE_EXPR: case ABS_EXPR: + case ABSU_EXPR: case TRUTH_NOT_EXPR: case FIXED_CONVERT_EXPR: case UNARY_PLUS_EXPR: diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c index b4e23e2..4567365 100644 --- a/gcc/cp/cp-gimplify.c +++ b/gcc/cp/cp-gimplify.c @@ -2272,6 +2272,7 @@ cp_fold (tree x) case FLOAT_EXPR: case NEGATE_EXPR: case ABS_EXPR: + case ABSU_EXPR: case BIT_NOT_EXPR: case TRUTH_NOT_EXPR: case FIXED_CONVERT_EXPR: diff --git a/gcc/dojump.c b/gcc/dojump.c index 9da8a0e..88cc96a 100644 --- a/gcc/dojump.c +++ b/gcc/dojump.c @@ -467,6 +467,7 @@ do_jump (tree exp, rtx_code_label *if_false_label, /* FALLTHRU */ case NON_LVALUE_EXPR: case ABS_EXPR: + case ABSU_EXPR: case NEGATE_EXPR: case LROTATE_EXPR: case RROTATE_EXPR: diff --git a/gcc/expr.c b/gcc/expr.c index ecc5292..a92c23f 100644 --- a/gcc/expr.c +++ b/gcc/expr.c @@ -9013,6 +9013,7 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode, return REDUCE_BIT_FIELD (temp); case ABS_EXPR: + case ABSU_EXPR: op0 = expand_expr (treeop0, subtarget, VOIDmode, EXPAND_NORMAL); if (modifier == EXPAND_STACK_PARM) @@ -9024,7 +9025,7 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode, /* Unsigned abs is simply the operand. Testing here means we don't risk generating incorrect code below. */ - if (TYPE_UNSIGNED (type)) + if (TYPE_UNSIGNED (TREE_TYPE (treeop0))) return op0; return expand_abs (mode, op0, target, unsignedp, diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 0f57f07..20b4271 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -1723,7 +1723,8 @@ const_unop (enum tree_code code, tree type, tree arg0) && HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg0))) && REAL_VALUE_ISSIGNALING_NAN (TREE_REAL_CST (arg0)) && code != NEGATE_EXPR - && code != ABS_EXPR) + && code != ABS_EXPR + && code != ABSU_EXPR) return NULL_TREE; switch (code) @@ -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: if (TREE_CODE (arg0) == COMPLEX_CST) diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c index 49e9e12..bb4d259 100644 --- a/gcc/gimple-pretty-print.c +++ b/gcc/gimple-pretty-print.c @@ -358,14 +358,17 @@ dump_unary_rhs (pretty_printer *buffer, gassign *gs, int spc, break; case ABS_EXPR: + case ABSU_EXPR: if (flags & TDF_GIMPLE) { - pp_string (buffer, "__ABS "); + pp_string (buffer, + rhs_code == ABS_EXPR ? "__ABS " : "__ABSU "); dump_generic_node (buffer, rhs, spc, flags, false); } else { - pp_string (buffer, "ABS_EXPR <"); + pp_string (buffer, + rhs_code == ABS_EXPR ? "ABS_EXPR <" : "ABSU_EXPR <"); dump_generic_node (buffer, rhs, spc, flags, false); pp_greater (buffer); } diff --git a/gcc/gimple-ssa-backprop.c b/gcc/gimple-ssa-backprop.c index 9ab655c..bb378a9 100644 --- a/gcc/gimple-ssa-backprop.c +++ b/gcc/gimple-ssa-backprop.c @@ -408,6 +408,7 @@ backprop::process_assign_use (gassign *assign, tree rhs, usage_info *info) switch (gimple_assign_rhs_code (assign)) { case ABS_EXPR: + case ABSU_EXPR: /* The sign of the input doesn't matter. */ info->flags.ignore_sign = true; break; @@ -675,6 +676,7 @@ strip_sign_op_1 (tree rhs) switch (gimple_assign_rhs_code (assign)) { case ABS_EXPR: + case ABSU_EXPR: case NEGATE_EXPR: return gimple_assign_rhs1 (assign); 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))))) + + /* Simplifications of operations with one constant operand and simplifications to constants or single values. */ diff --git a/gcc/optabs-tree.c b/gcc/optabs-tree.c index 73e6654..092663b 100644 --- a/gcc/optabs-tree.c +++ b/gcc/optabs-tree.c @@ -234,6 +234,8 @@ optab_for_tree_code (enum tree_code code, const_tree type, case ABS_EXPR: return trapv ? absv_optab : abs_optab; + case ABSU_EXPR: + return abs_optab; default: return unknown_optab; } diff --git a/gcc/testsuite/gcc.dg/absu.c b/gcc/testsuite/gcc.dg/absu.c new file mode 100644 index 0000000..81e37a4 --- /dev/null +++ b/gcc/testsuite/gcc.dg/absu.c @@ -0,0 +1,41 @@ + +/* { dg-do run } */ +/* { dg-options "-O0" } */ + +#include <limits.h> +#define ABS(x) (((x) >= 0) ? (x) : -(x)) + +#define DEF_TEST(TYPE) \ +void foo_##TYPE (signed TYPE x, unsigned TYPE y){ \ + TYPE t = ABS (x); \ + if (t != y) \ + __builtin_abort (); \ +} \ + +DEF_TEST (char); +DEF_TEST (short); +DEF_TEST (int); +DEF_TEST (long); +void main () +{ + foo_char (SCHAR_MIN + 1, SCHAR_MAX); + foo_char (0, 0); + foo_char (-1, 1); + foo_char (1, 1); + foo_char (SCHAR_MAX, SCHAR_MAX); + + foo_int (-1, 1); + foo_int (0, 0); + foo_int (INT_MAX, INT_MAX); + foo_int (INT_MIN + 1, INT_MAX); + + foo_short (-1, 1); + foo_short (0, 0); + foo_short (SHRT_MAX, SHRT_MAX); + foo_short (SHRT_MIN + 1, SHRT_MAX); + + foo_long (-1, 1); + foo_long (0, 0); + foo_long (LONG_MAX, LONG_MAX); + foo_long (LONG_MIN + 1, LONG_MAX); +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-29.c b/gcc/testsuite/gcc.dg/gimplefe-29.c new file mode 100644 index 0000000..54b86ef --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-29.c @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-O -fgimple -fdump-tree-ssa-gimple" } */ + +unsigned int __GIMPLE() f(int a) +{ + unsigned int t0; + t0_1 = __ABSU a; + return t0_1; +} + +/* { dg-final { scan-tree-dump "__ABSU a" "ssa" } } */ 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)) + return true; + return false; + break; case VEC_DUPLICATE_EXPR: if (TREE_CODE (lhs_type) != VECTOR_TYPE 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) diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 7881131..665e53c 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -3865,6 +3865,7 @@ estimate_operator_cost (enum tree_code code, eni_weights *weights, case MIN_EXPR: case MAX_EXPR: case ABS_EXPR: + case ABSU_EXPR: case LSHIFT_EXPR: case RSHIFT_EXPR: diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c index 5a8c8eb..a950a9e 100644 --- a/gcc/tree-pretty-print.c +++ b/gcc/tree-pretty-print.c @@ -2463,6 +2463,12 @@ dump_generic_node (pretty_printer *pp, tree node, int spc, dump_flags_t flags, pp_greater (pp); break; + case ABSU_EXPR: + pp_string (pp, "ABSU_EXPR <"); + dump_generic_node (pp, TREE_OPERAND (node, 0), spc, flags, false); + pp_greater (pp); + break; + case RANGE_EXPR: NIY; break; diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c index 6da784c..af6b9bd 100644 --- a/gcc/tree-vect-patterns.c +++ b/gcc/tree-vect-patterns.c @@ -615,7 +615,8 @@ vect_recog_sad_pattern (vec<gimple *> *stmts, tree *type_in, gcc_assert (abs_stmt_vinfo); if (STMT_VINFO_DEF_TYPE (abs_stmt_vinfo) != vect_internal_def) return NULL; - if (gimple_assign_rhs_code (abs_stmt) != ABS_EXPR) + if (gimple_assign_rhs_code (abs_stmt) != ABS_EXPR + && gimple_assign_rhs_code (abs_stmt) != ABSU_EXPR) return NULL; tree abs_oprnd = gimple_assign_rhs1 (abs_stmt); 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 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
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.
Hi Richard, Thanks for the review. On 1 June 2018 at 22:20, Richard Biener <richard.guenther@gmail.com> wrote: > 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). The Gimple FE testcase is running into this. > > @@ -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; > } > > + 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. > > 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.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. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 859eeb4..0e8efb5 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -3312,6 +3312,7 @@ c_common_truthvalue_conversion (location_t location, tree expr) case NEGATE_EXPR: case ABS_EXPR: + case ABSU_EXPR: case FLOAT_EXPR: case EXCESS_PRECISION_EXPR: /* These don't change whether an object is nonzero or zero. */ diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 45a4529..5bb6804 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -4314,6 +4314,16 @@ build_unary_op (location_t location, enum tree_code code, tree xarg, arg = default_conversion (arg); break; + case ABSU_EXPR: + if (!(typecode == INTEGER_TYPE)) + { + error_at (location, "wrong type argument to absu"); + return error_mark_node; + } + else if (!noconvert) + arg = default_conversion (arg); + break; + case CONJ_EXPR: /* Conjugating a real value is a no-op, but allow it anyway. */ if (!(typecode == INTEGER_TYPE || typecode == REAL_TYPE diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index c9abe24..11e76ff 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -328,7 +328,8 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq) case CPP_NAME: { tree id = c_parser_peek_token (parser)->value; - if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0) + if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0 + || strcmp (IDENTIFIER_POINTER (id), "__ABSU") == 0) goto build_unary_expr; break; } @@ -638,6 +639,12 @@ c_parser_gimple_unary_expression (c_parser *parser) op = c_parser_gimple_postfix_expression (parser); return parser_build_unary_op (op_loc, ABS_EXPR, op); } + else if (strcmp (IDENTIFIER_POINTER (id), "__ABSU") == 0) + { + c_parser_consume_token (parser); + op = c_parser_gimple_postfix_expression (parser); + return parser_build_unary_op (op_loc, ABSU_EXPR, op); + } else return c_parser_gimple_postfix_expression (parser); } diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index ef143a3..ba4543c 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -4621,6 +4621,7 @@ expand_debug_expr (tree exp) } case ABS_EXPR: + case ABSU_EXPR: return simplify_gen_unary (ABS, mode, op0, mode); case NEGATE_EXPR: diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index e19864d..487127c 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -51228,6 +51228,7 @@ ix86_add_stmt_cost (void *data, int count, enum vect_cost_for_stmt kind, case BIT_IOR_EXPR: case ABS_EXPR: + case ABSU_EXPR: case MIN_EXPR: case MAX_EXPR: case BIT_XOR_EXPR: diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 8c6ec55..bea9147 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -5760,6 +5760,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now, case FLOAT_EXPR: case NEGATE_EXPR: case ABS_EXPR: + case ABSU_EXPR: case TRUTH_NOT_EXPR: case FIXED_CONVERT_EXPR: case UNARY_PLUS_EXPR: diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c index eda5f05..bca0c59 100644 --- a/gcc/cp/cp-gimplify.c +++ b/gcc/cp/cp-gimplify.c @@ -2271,6 +2271,7 @@ cp_fold (tree x) case FLOAT_EXPR: case NEGATE_EXPR: case ABS_EXPR: + case ABSU_EXPR: case BIT_NOT_EXPR: case TRUTH_NOT_EXPR: case FIXED_CONVERT_EXPR: diff --git a/gcc/dojump.c b/gcc/dojump.c index 9da8a0e..88cc96a 100644 --- a/gcc/dojump.c +++ b/gcc/dojump.c @@ -467,6 +467,7 @@ do_jump (tree exp, rtx_code_label *if_false_label, /* FALLTHRU */ case NON_LVALUE_EXPR: case ABS_EXPR: + case ABSU_EXPR: case NEGATE_EXPR: case LROTATE_EXPR: case RROTATE_EXPR: diff --git a/gcc/expr.c b/gcc/expr.c index 00a802c..9efa535 100644 --- a/gcc/expr.c +++ b/gcc/expr.c @@ -9074,6 +9074,7 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode, return REDUCE_BIT_FIELD (temp); case ABS_EXPR: + case ABSU_EXPR: op0 = expand_expr (treeop0, subtarget, VOIDmode, EXPAND_NORMAL); if (modifier == EXPAND_STACK_PARM) @@ -9085,7 +9086,7 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode, /* Unsigned abs is simply the operand. Testing here means we don't risk generating incorrect code below. */ - if (TYPE_UNSIGNED (type)) + if (TYPE_UNSIGNED (TREE_TYPE (treeop0))) return op0; return expand_abs (mode, op0, target, unsignedp, diff --git a/gcc/fold-const.c b/gcc/fold-const.c index faa184a..c19614e 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -1723,7 +1723,8 @@ const_unop (enum tree_code code, tree type, tree arg0) && HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg0))) && REAL_VALUE_ISSIGNALING_NAN (TREE_REAL_CST (arg0)) && code != NEGATE_EXPR - && code != ABS_EXPR) + && code != ABS_EXPR + && code != ABSU_EXPR) return NULL_TREE; switch (code) @@ -1758,6 +1759,7 @@ const_unop (enum tree_code code, tree type, tree arg0) } case ABS_EXPR: + case ABSU_EXPR: if (TREE_CODE (arg0) == INTEGER_CST || TREE_CODE (arg0) == REAL_CST) return fold_abs_const (arg0, type); break; @@ -13846,20 +13848,21 @@ 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)); - } + 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; + break; case REAL_CST: if (REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg0))) diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c index afe0147..4fa992d 100644 --- a/gcc/gimple-pretty-print.c +++ b/gcc/gimple-pretty-print.c @@ -358,14 +358,17 @@ dump_unary_rhs (pretty_printer *buffer, gassign *gs, int spc, break; case ABS_EXPR: + case ABSU_EXPR: if (flags & TDF_GIMPLE) { - pp_string (buffer, "__ABS "); + pp_string (buffer, + rhs_code == ABS_EXPR ? "__ABS " : "__ABSU "); dump_generic_node (buffer, rhs, spc, flags, false); } else { - pp_string (buffer, "ABS_EXPR <"); + pp_string (buffer, + rhs_code == ABS_EXPR ? "ABS_EXPR <" : "ABSU_EXPR <"); dump_generic_node (buffer, rhs, spc, flags, false); pp_greater (buffer); } diff --git a/gcc/gimple-ssa-backprop.c b/gcc/gimple-ssa-backprop.c index 27aa575..a38b5eb 100644 --- a/gcc/gimple-ssa-backprop.c +++ b/gcc/gimple-ssa-backprop.c @@ -405,6 +405,7 @@ backprop::process_assign_use (gassign *assign, tree rhs, usage_info *info) switch (gimple_assign_rhs_code (assign)) { case ABS_EXPR: + case ABSU_EXPR: /* The sign of the input doesn't matter. */ info->flags.ignore_sign = true; break; @@ -681,6 +682,7 @@ strip_sign_op_1 (tree rhs) switch (gimple_assign_rhs_code (assign)) { case ABS_EXPR: + case ABSU_EXPR: case NEGATE_EXPR: return gimple_assign_rhs1 (assign); diff --git a/gcc/match.pd b/gcc/match.pd index 7033730..ba52bb0 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -90,6 +90,17 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (match (nop_convert @0) @0) +/* Transform likes of (char) ABS_EXPR <(int) x> into (char) ABSU_EXPR <x> + ABSU_EXPR returns unsigned absolute value of the operand and the operand + of the ABSU_EXPR will have the corresponding signed type. */ +(simplify (abs (convert @0)) + (if (ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0)) + && !TYPE_UNSIGNED (TREE_TYPE (@0)) + && element_precision (type) > element_precision (TREE_TYPE (@0))) + (with { tree utype = unsigned_type_for (TREE_TYPE (@0)); } + (convert (absu:utype @0))))) + + /* Simplifications of operations with one constant operand and simplifications to constants or single values. */ diff --git a/gcc/optabs-tree.c b/gcc/optabs-tree.c index 71e172c..aa119ec 100644 --- a/gcc/optabs-tree.c +++ b/gcc/optabs-tree.c @@ -237,6 +237,8 @@ optab_for_tree_code (enum tree_code code, const_tree type, case ABS_EXPR: return trapv ? absv_optab : abs_optab; + case ABSU_EXPR: + return abs_optab; default: return unknown_optab; } diff --git a/gcc/testsuite/gcc.dg/absu.c b/gcc/testsuite/gcc.dg/absu.c index e69de29..063da28 100644 --- a/gcc/testsuite/gcc.dg/absu.c +++ b/gcc/testsuite/gcc.dg/absu.c @@ -0,0 +1,85 @@ + +/* { dg-do run } */ +/* { dg-options "-O0" } */ + +#include <limits.h> +#define ABS(x) (((x) >= 0) ? (x) : -(x)) + +#define DEF_TEST(TYPE) \ +void foo_##TYPE (signed TYPE x, unsigned TYPE y){ \ + TYPE t = ABS (x); \ + if (t != y) \ + __builtin_abort (); \ +} \ + +DEF_TEST (char); +DEF_TEST (short); +DEF_TEST (int); +DEF_TEST (long); + +int main () +{ + foo_char (SCHAR_MIN + 1, SCHAR_MAX); + foo_char (0, 0); + foo_char (-1, 1); + foo_char (1, 1); + foo_char (SCHAR_MAX, SCHAR_MAX); + + foo_int (-1, 1); + foo_int (0, 0); + foo_int (INT_MAX, INT_MAX); + foo_int (INT_MIN + 1, INT_MAX); + + foo_short (-1, 1); + foo_short (0, 0); + foo_short (SHRT_MAX, SHRT_MAX); + foo_short (SHRT_MIN + 1, SHRT_MAX); + + foo_long (-1, 1); + foo_long (0, 0); + foo_long (LONG_MAX, LONG_MAX); + foo_long (LONG_MIN + 1, LONG_MAX); + + return 0; +} + +/* { dg-do run } */ +/* { dg-options "-O0" } */ + +#include <limits.h> +#define ABS(x) (((x) >= 0) ? (x) : -(x)) + +#define DEF_TEST(TYPE) \ +void foo_##TYPE (signed TYPE x, unsigned TYPE y){ \ + TYPE t = ABS (x); \ + if (t != y) \ + __builtin_abort (); \ +} \ + +DEF_TEST (char); +DEF_TEST (short); +DEF_TEST (int); +DEF_TEST (long); +void main () +{ + foo_char (SCHAR_MIN + 1, SCHAR_MAX); + foo_char (0, 0); + foo_char (-1, 1); + foo_char (1, 1); + foo_char (SCHAR_MAX, SCHAR_MAX); + + foo_int (-1, 1); + foo_int (0, 0); + foo_int (INT_MAX, INT_MAX); + foo_int (INT_MIN + 1, INT_MAX); + + foo_short (-1, 1); + foo_short (0, 0); + foo_short (SHRT_MAX, SHRT_MAX); + foo_short (SHRT_MIN + 1, SHRT_MAX); + + foo_long (-1, 1); + foo_long (0, 0); + foo_long (LONG_MAX, LONG_MAX); + foo_long (LONG_MIN + 1, LONG_MAX); +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-29.c b/gcc/testsuite/gcc.dg/gimplefe-29.c index e69de29..54b86ef 100644 --- a/gcc/testsuite/gcc.dg/gimplefe-29.c +++ b/gcc/testsuite/gcc.dg/gimplefe-29.c @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-O -fgimple -fdump-tree-ssa-gimple" } */ + +unsigned int __GIMPLE() f(int a) +{ + unsigned int t0; + t0_1 = __ABSU a; + return t0_1; +} + +/* { dg-final { scan-tree-dump "__ABSU a" "ssa" } } */ diff --git a/gcc/testsuite/gcc.target/aarch64/pr64946.c b/gcc/testsuite/gcc.target/aarch64/pr64946.c index e69de29..736656f 100644 --- a/gcc/testsuite/gcc.target/aarch64/pr64946.c +++ b/gcc/testsuite/gcc.target/aarch64/pr64946.c @@ -0,0 +1,13 @@ + +/* { dg-do compile } */ +/* { dg-options "-O3" } */ + +signed char a[100],b[100]; +void absolute_s8 (void) +{ + int i; + for (i=0; i<16; i++) + a[i] = (b[i] > 0 ? b[i] : -b[i]); +}; + +/* { dg-final { scan-assembler-times "abs\tv\[0-9\]+.16b, v\[0-9\]+.16b" 1 } } */ diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index 7f48d2d..6df42cc 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)) + return true; + return false; + break; case VEC_DUPLICATE_EXPR: if (TREE_CODE (lhs_type) != VECTOR_TYPE diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c index 3609bca..da87466 100644 --- a/gcc/tree-eh.c +++ b/gcc/tree-eh.c @@ -2471,6 +2471,10 @@ operation_could_trap_helper_p (enum tree_code op, return true; return false; + case ABSU_EXPR: + /* ABSU_EXPR never traps. */ + return false; + case PLUS_EXPR: case MINUS_EXPR: case MULT_EXPR: diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 5a0a252..d272974 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -3866,6 +3866,7 @@ estimate_operator_cost (enum tree_code code, eni_weights *weights, case MIN_EXPR: case MAX_EXPR: case ABS_EXPR: + case ABSU_EXPR: case LSHIFT_EXPR: case RSHIFT_EXPR: diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c index bc36c28..612a18f 100644 --- a/gcc/tree-pretty-print.c +++ b/gcc/tree-pretty-print.c @@ -2463,6 +2463,12 @@ dump_generic_node (pretty_printer *pp, tree node, int spc, dump_flags_t flags, pp_greater (pp); break; + case ABSU_EXPR: + pp_string (pp, "ABSU_EXPR <"); + dump_generic_node (pp, TREE_OPERAND (node, 0), spc, flags, false); + pp_greater (pp); + break; + case RANGE_EXPR: NIY; break; diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c index 5c2578f..e4df6c8 100644 --- a/gcc/tree-vect-patterns.c +++ b/gcc/tree-vect-patterns.c @@ -614,7 +614,8 @@ vect_recog_sad_pattern (vec<gimple *> *stmts, tree *type_in, gcc_assert (abs_stmt_vinfo); if (STMT_VINFO_DEF_TYPE (abs_stmt_vinfo) != vect_internal_def) return NULL; - if (gimple_assign_rhs_code (abs_stmt) != ABS_EXPR) + if (gimple_assign_rhs_code (abs_stmt) != ABS_EXPR + && gimple_assign_rhs_code (abs_stmt) != ABSU_EXPR) return NULL; tree abs_oprnd = gimple_assign_rhs1 (abs_stmt); diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c index 4539f6a..c71b688 100644 --- a/gcc/tree-vect-stmts.c +++ b/gcc/tree-vect-stmts.c @@ -5995,7 +5995,10 @@ 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, vectype_out); + 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 diff --git a/gcc/tree.def b/gcc/tree.def index 31de6c0..a1766e4 100644 --- a/gcc/tree.def +++ b/gcc/tree.def @@ -761,6 +761,11 @@ DEFTREECODE (MAX_EXPR, "max_expr", tcc_binary, 2) operand of the ABS_EXPR must have the same type. */ DEFTREECODE (ABS_EXPR, "abs_expr", tcc_unary, 1) +/* 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. */ +DEFTREECODE (ABSU_EXPR, "absu_expr", tcc_unary, 1) + /* Shift operations for shift and rotate. Shift means logical shift if done on an unsigned type, arithmetic shift if done on a signed type.
On Mon, Jun 4, 2018 at 10:18 AM Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> wrote: > > Hi Richard, > > Thanks for the review. > > On 1 June 2018 at 22:20, Richard Biener <richard.guenther@gmail.com> wrote: > > 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). > > 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.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.
Hi Richard, Thanks for the review and sorry for getting back to you late. On 4 June 2018 at 18:38, Richard Biener <richard.guenther@gmail.com> wrote: > On Mon, Jun 4, 2018 at 10:18 AM Kugan Vivekanandarajah > <kugan.vivekanandarajah@linaro.org> wrote: >> >> Hi Richard, >> >> Thanks for the review. >> >> On 1 June 2018 at 22:20, Richard Biener <richard.guenther@gmail.com> wrote: >> > 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). >> >> 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); Some testcases are failing with the changes. gcc:gcc.dg/tree-ssa/pr83329.c (internal compiler error) : This seems to be due to POINTER_DIFF_EXPR (?) There is also gcc:gcc.target/i386/avx2-vshift-1.c (internal compiler error) gcc:gcc.target/i386/xop-vshift-1.c (internal compiler error) Example: /home/tcwg-buildslave/workspace/tcwg-buildfarm_2/tcwg-x86_32-build/snapshots/gcc.git~master_rev_bfa6b77/gcc/testsuite/gcc.target/i386/xop-vshift-1.c:73:1: error: type mismatch in binary expression^M vector(4) long long unsigned int^M ^M vector(4) long long int^M ^M vector(4) long long int^M ^M vect_patt_17.54_21 = vect__2.53_16 & vect_cst__20;^M during GIMPLE pass: vect^M Thanks, Kugan > > 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.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.
On Mon, Jun 11, 2018 at 10:28 AM Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> wrote: > > Hi Richard, > > Thanks for the review and sorry for getting back to you late. > > On 4 June 2018 at 18:38, Richard Biener <richard.guenther@gmail.com> wrote: > > On Mon, Jun 4, 2018 at 10:18 AM Kugan Vivekanandarajah > > <kugan.vivekanandarajah@linaro.org> wrote: > >> > >> Hi Richard, > >> > >> Thanks for the review. > >> > >> On 1 June 2018 at 22:20, Richard Biener <richard.guenther@gmail.com> wrote: > >> > 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). > >> > >> 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); > > Some testcases are failing with the changes. > > gcc:gcc.dg/tree-ssa/pr83329.c (internal compiler error) : This seems > to be due to POINTER_DIFF_EXPR (?) Ah, yes. I'd simply handle that in a more clear way. > There is also > > gcc:gcc.target/i386/avx2-vshift-1.c (internal compiler error) > gcc:gcc.target/i386/xop-vshift-1.c (internal compiler error) > > Example: > /home/tcwg-buildslave/workspace/tcwg-buildfarm_2/tcwg-x86_32-build/snapshots/gcc.git~master_rev_bfa6b77/gcc/testsuite/gcc.target/i386/xop-vshift-1.c:73:1: > error: type mismatch in binary expression^M > vector(4) long long unsigned int^M > ^M > vector(4) long long int^M > ^M > vector(4) long long int^M > ^M > vect_patt_17.54_21 = vect__2.53_16 & vect_cst__20;^M > during GIMPLE pass: vect^M That one is odd and looks like a latent bug in pattern creation: diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c index 507c5b94f07..6786ffcd4c6 100644 --- a/gcc/tree-vect-patterns.c +++ b/gcc/tree-vect-patterns.c @@ -2185,6 +2185,11 @@ vect_recog_vector_vector_shift_pattern (vec<gimple *> *stmts, TYPE_PRECISION (TREE_TYPE (oprnd1))); def = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL); def_stmt = gimple_build_assign (def, BIT_AND_EXPR, rhs1, mask); + stmt_vec_info new_stmt_info + = new_stmt_vec_info (def_stmt, vinfo); + set_vinfo_for_stmt (def_stmt, new_stmt_info); + STMT_VINFO_VECTYPE (new_stmt_info) + = get_vectype_for_scalar_type (TREE_TYPE (rhs1)); new_pattern_def_seq (stmt_vinfo, def_stmt); } } I can see if I can make this change independently of yours (that is, I'm bootstrapping and testing the above together with an equivalent vectorizable_operation hunk). Richard. > > Thanks, > Kugan > > > > > > > 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.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.
diff --git a/gcc/expr.c b/gcc/expr.c index 5e3d9a5..67f8dd1 100644 --- a/gcc/expr.c +++ b/gcc/expr.c @@ -9063,6 +9063,7 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode, return REDUCE_BIT_FIELD (temp); case ABS_EXPR: + case ABSU_EXPR: op0 = expand_expr (treeop0, subtarget, VOIDmode, EXPAND_NORMAL); if (modifier == EXPAND_STACK_PARM) @@ -9074,7 +9075,8 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode, /* 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; return expand_abs (mode, op0, target, unsignedp, diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 3a99b66..6e80178 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -5324,8 +5324,17 @@ fold_cond_expr_with_comparison (location_t loc, tree type, case GT_EXPR: if (TYPE_UNSIGNED (TREE_TYPE (arg1))) break; - tem = fold_build1_loc (loc, ABS_EXPR, TREE_TYPE (arg1), arg1); - return fold_convert_loc (loc, type, tem); + if (TREE_CODE (arg1) == NOP_EXPR) + { + arg1 = TREE_OPERAND (arg1, 0); + tem = fold_build1_loc (loc, ABSU_EXPR, unsigned_type_for (arg1_type), arg1); + return fold_convert_loc (loc, type, tem); + } + else + { + tem = fold_build1_loc (loc, ABS_EXPR, TREE_TYPE (arg1), arg1); + return fold_convert_loc (loc, type, tem); + } case UNLE_EXPR: case UNLT_EXPR: if (flag_trapping_math) @@ -7698,7 +7707,8 @@ fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) if (arg0) { if (CONVERT_EXPR_CODE_P (code) - || code == FLOAT_EXPR || code == ABS_EXPR || code == NEGATE_EXPR) + || code == FLOAT_EXPR || code == ABS_EXPR + || code == ABSU_EXPR || code == NEGATE_EXPR) { /* Don't use STRIP_NOPS, because signedness of argument type matters. */ 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; default: diff --git a/gcc/testsuite/gcc.dg/absu.c b/gcc/testsuite/gcc.dg/absu.c index e69de29..43e651b 100644 --- a/gcc/testsuite/gcc.dg/absu.c +++ b/gcc/testsuite/gcc.dg/absu.c @@ -0,0 +1,39 @@ + +/* { dg-do run } */ +/* { dg-options "-O0" } */ + +#include <limits.h> +#define ABS(x) (((x) >= 0) ? (x) : -(x)) + +#define DEF_TEST(TYPE) \ +void foo_##TYPE (signed TYPE x, unsigned TYPE y){ \ + TYPE t = ABS (x); \ + if (t != y) \ + __builtin_abort (); \ +} \ + +DEF_TEST (char); +DEF_TEST (short); +DEF_TEST (int); +DEF_TEST (long); +void main () +{ + foo_char (SCHAR_MIN + 1, SCHAR_MAX); + foo_char (0, 0); + foo_char (SCHAR_MAX, SCHAR_MAX); + + foo_int (-1, 1); + foo_int (0, 0); + foo_int (INT_MAX, INT_MAX); + foo_int (INT_MIN + 1, INT_MAX); + + foo_short (-1, 1); + foo_short (0, 0); + foo_short (SHRT_MAX, SHRT_MAX); + foo_short (SHRT_MIN + 1, SHRT_MAX); + + foo_long (-1, 1); + foo_long (0, 0); + foo_long (LONG_MAX, LONG_MAX); + foo_long (LONG_MIN + 1, LONG_MAX); +} diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index 9485f73..59a115c 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -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: case BIT_FIELD_REF: @@ -3937,6 +3940,9 @@ verify_gimple_assign_unary (gassign *stmt) case PAREN_EXPR: case CONJ_EXPR: break; + case ABSU_EXPR: + /* FIXME. */ + return false; case VEC_DUPLICATE_EXPR: if (TREE_CODE (lhs_type) != VECTOR_TYPE diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c index 71dac4f..13d4c25 100644 --- a/gcc/tree-if-conv.c +++ b/gcc/tree-if-conv.c @@ -466,7 +466,8 @@ fold_build_cond_expr (tree type, tree cond, tree rhs, tree lhs) 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); diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 5a0a252..d272974 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -3866,6 +3866,7 @@ estimate_operator_cost (enum tree_code code, eni_weights *weights, case MIN_EXPR: case MAX_EXPR: case ABS_EXPR: + case ABSU_EXPR: case LSHIFT_EXPR: case RSHIFT_EXPR: diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c index 276ad00..b74d8c9 100644 --- a/gcc/tree-pretty-print.c +++ b/gcc/tree-pretty-print.c @@ -2460,6 +2460,12 @@ dump_generic_node (pretty_printer *pp, tree node, int spc, dump_flags_t flags, pp_greater (pp); break; + case ABSU_EXPR: + pp_string (pp, "ABSU_EXPR <"); + dump_generic_node (pp, TREE_OPERAND (node, 0), spc, flags, false); + pp_greater (pp); + break; + case RANGE_EXPR: NIY; break; diff --git a/gcc/tree.def b/gcc/tree.def index 31de6c0..768167b 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
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. 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.