| Message ID | CAHFci2_AQDsRFnAJ5qt7EvGTuU643gMXjoyiDtRnig+qcOQ2Xg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 23, 2016 at 3:57 PM, Bin.Cheng <amker.cheng@gmail.com> wrote: > On Wed, Nov 23, 2016 at 2:27 PM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Wed, Nov 23, 2016 at 2:54 PM, Bin Cheng <Bin.Cheng@arm.com> wrote: >>> Hi, >>> This is actually the review suggestion for patch @https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02341.html, but I forgot to incorporate it when committing that patch. Here comes this one doing that, as well as adding a missing convert keyword. Toolchain built successfully, is it OK? >> >> As said you _do_ need the outermost (convert ...) on the (max .. and >> (min ... expressions given @1 may not be of type 'type'. > Sorry about the stupid mistake. How about this one? The from_type in > the last branch looks like necessary to me. I think (if (code == EQ_EXPR) (cond (cmp @1 (convert @3)) (convert @3) @2))))))) is better? We want the outer expression of type 'type' and @2 is already 'type', only @3 may not be. So the only change would be to dop the unnecessary :from_type inside the cmp and the bogus :from_type on the true arg of the cond. Richard. > Thanks, > bin >> >>> Thanks, >>> bin >>> >>> 2016-11-23 Bin Cheng <bin.cheng@arm.com> >>> >>> * match.pd: Refine type conversion in result expressions for below >>> pattern: >>> (cond (cmp (convert1? x) c1) (convert2? x) c2) -> (minmax (x c)).
On Thu, Nov 24, 2016 at 8:57 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Wed, Nov 23, 2016 at 3:57 PM, Bin.Cheng <amker.cheng@gmail.com> wrote: >> On Wed, Nov 23, 2016 at 2:27 PM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Wed, Nov 23, 2016 at 2:54 PM, Bin Cheng <Bin.Cheng@arm.com> wrote: >>>> Hi, >>>> This is actually the review suggestion for patch @https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02341.html, but I forgot to incorporate it when committing that patch. Here comes this one doing that, as well as adding a missing convert keyword. Toolchain built successfully, is it OK? >>> >>> As said you _do_ need the outermost (convert ...) on the (max .. and >>> (min ... expressions given @1 may not be of type 'type'. >> Sorry about the stupid mistake. How about this one? The from_type in >> the last branch looks like necessary to me. > > I think > > (if (code == EQ_EXPR) > (cond (cmp @1 (convert @3)) (convert @3) @2))))))) > > is better? We want the outer expression of type 'type' and @2 is > already 'type', > only @3 may not be. So the only change would be to dop the unnecessary > :from_type inside the cmp and the bogus :from_type on the true arg of the cond. Hi Richard, The idea of using from_type in EQ_EXPR case is to do cond_expr in narrow/from type for all its operands, then convert the result back to default type. - (cond (cmp @1 (convert:from_type @3)) (convert:from_type @3) @2))))))) + (convert (cond (cmp @1 (convert @3)) + (convert:from_type @3) (convert:from_type @2))))))))) Is it better than using different types for operand 0 and 1/2 in cond_expr? I updated the patch following your suggestion. Note, in this way below range check on @2 should be redundant for EQ_EXPR case, but I didn't change that in this patch. if (int_fits_type_p (@2, from_type) && (types_match (c1_type, from_type) || (TYPE_PRECISION (c1_type) > TYPE_PRECISION (from_type) && (TYPE_UNSIGNED (from_type) || TYPE_SIGN (c1_type) == TYPE_SIGN (from_type)))) So which one should be preferred? Thanks, bin > > Richard. > > >> Thanks, >> bin >>> >>>> Thanks, >>>> bin >>>> >>>> 2016-11-23 Bin Cheng <bin.cheng@arm.com> >>>> >>>> * match.pd: Refine type conversion in result expressions for below >>>> pattern: >>>> (cond (cmp (convert1? x) c1) (convert2? x) c2) -> (minmax (x c)).
On Thu, Nov 24, 2016 at 11:11 AM, Bin.Cheng <amker.cheng@gmail.com> wrote: > On Thu, Nov 24, 2016 at 8:57 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Wed, Nov 23, 2016 at 3:57 PM, Bin.Cheng <amker.cheng@gmail.com> wrote: >>> On Wed, Nov 23, 2016 at 2:27 PM, Richard Biener >>> <richard.guenther@gmail.com> wrote: >>>> On Wed, Nov 23, 2016 at 2:54 PM, Bin Cheng <Bin.Cheng@arm.com> wrote: >>>>> Hi, >>>>> This is actually the review suggestion for patch @https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02341.html, but I forgot to incorporate it when committing that patch. Here comes this one doing that, as well as adding a missing convert keyword. Toolchain built successfully, is it OK? >>>> >>>> As said you _do_ need the outermost (convert ...) on the (max .. and >>>> (min ... expressions given @1 may not be of type 'type'. >>> Sorry about the stupid mistake. How about this one? The from_type in >>> the last branch looks like necessary to me. >> >> I think >> >> (if (code == EQ_EXPR) >> (cond (cmp @1 (convert @3)) (convert @3) @2))))))) >> >> is better? We want the outer expression of type 'type' and @2 is >> already 'type', >> only @3 may not be. So the only change would be to dop the unnecessary >> :from_type inside the cmp and the bogus :from_type on the true arg of the cond. > Hi Richard, > The idea of using from_type in EQ_EXPR case is to do cond_expr in > narrow/from type for all its operands, then convert the result back to > default type. I see. > - (cond (cmp @1 (convert:from_type @3)) (convert:from_type @3) @2))))))) > + (convert (cond (cmp @1 (convert @3)) > + (convert:from_type @3) (convert:from_type @2))))))))) > > Is it better than using different types for operand 0 and 1/2 in cond_expr? Ah, that's a valid point... > I updated the patch following your suggestion. Note, in this way > below range check on @2 should be redundant for EQ_EXPR case, but I > didn't change that in this patch. > > if (int_fits_type_p (@2, from_type) > && (types_match (c1_type, from_type) > || (TYPE_PRECISION (c1_type) > TYPE_PRECISION (from_type) > && (TYPE_UNSIGNED (from_type) > || TYPE_SIGN (c1_type) == TYPE_SIGN (from_type)))) > > So which one should be preferred? I suppose it's better to use the same type and thus your version then (-20161123). That patch is ok. Note my worry here is usually that we already have conflicting foldings in this area (moving conversions in/out), see fold_unary: /* If this was a conversion, and all we did was to move into inside the COND_EXPR, bring it back out. But leave it if it is a conversion from integer to integer and the result precision is no wider than a word since such a conversion is cheap and may be optimized away by combine, while it couldn't if it were outside the COND_EXPR. Then return so we don't get into an infinite recursion loop taking the conversion out and then back in. */ if ((CONVERT_EXPR_CODE_P (code) || code == NON_LVALUE_EXPR) && TREE_CODE (tem) == COND_EXPR && TREE_CODE (TREE_OPERAND (tem, 1)) == code && TREE_CODE (TREE_OPERAND (tem, 2)) == code && ! VOID_TYPE_P (TREE_OPERAND (tem, 1)) && ! VOID_TYPE_P (TREE_OPERAND (tem, 2)) && (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 1), 0)) == TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 2), 0))) && (! (INTEGRAL_TYPE_P (TREE_TYPE (tem)) && (INTEGRAL_TYPE_P (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 1), 0)))) && TYPE_PRECISION (TREE_TYPE (tem)) <= BITS_PER_WORD) || flag_syntax_only)) tem = build1_loc (loc, code, type, build3 (COND_EXPR, TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 1), 0)), TREE_OPERAND (tem, 0), TREE_OPERAND (TREE_OPERAND (tem, 1), 0), TREE_OPERAND (TREE_OPERAND (tem, 2), 0))); and fold_ternary has quite a bit of COND_EXPR folding as well. Thanks, Richard. > > Thanks, > bin >> >> Richard. >> >> >>> Thanks, >>> bin >>>> >>>>> Thanks, >>>>> bin >>>>> >>>>> 2016-11-23 Bin Cheng <bin.cheng@arm.com> >>>>> >>>>> * match.pd: Refine type conversion in result expressions for below >>>>> pattern: >>>>> (cond (cmp (convert1? x) c1) (convert2? x) c2) -> (minmax (x c)).
On Thu, Nov 24, 2016 at 11:17 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Nov 24, 2016 at 11:11 AM, Bin.Cheng <amker.cheng@gmail.com> wrote: >> On Thu, Nov 24, 2016 at 8:57 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Wed, Nov 23, 2016 at 3:57 PM, Bin.Cheng <amker.cheng@gmail.com> wrote: >>>> On Wed, Nov 23, 2016 at 2:27 PM, Richard Biener >>>> <richard.guenther@gmail.com> wrote: >>>>> On Wed, Nov 23, 2016 at 2:54 PM, Bin Cheng <Bin.Cheng@arm.com> wrote: >>>>>> Hi, >>>>>> This is actually the review suggestion for patch @https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02341.html, but I forgot to incorporate it when committing that patch. Here comes this one doing that, as well as adding a missing convert keyword. Toolchain built successfully, is it OK? >>>>> >>>>> As said you _do_ need the outermost (convert ...) on the (max .. and >>>>> (min ... expressions given @1 may not be of type 'type'. >>>> Sorry about the stupid mistake. How about this one? The from_type in >>>> the last branch looks like necessary to me. >>> >>> I think >>> >>> (if (code == EQ_EXPR) >>> (cond (cmp @1 (convert @3)) (convert @3) @2))))))) >>> >>> is better? We want the outer expression of type 'type' and @2 is >>> already 'type', >>> only @3 may not be. So the only change would be to dop the unnecessary >>> :from_type inside the cmp and the bogus :from_type on the true arg of the cond. >> Hi Richard, >> The idea of using from_type in EQ_EXPR case is to do cond_expr in >> narrow/from type for all its operands, then convert the result back to >> default type. > > I see. > >> - (cond (cmp @1 (convert:from_type @3)) (convert:from_type @3) @2))))))) >> + (convert (cond (cmp @1 (convert @3)) >> + (convert:from_type @3) (convert:from_type @2))))))))) >> >> Is it better than using different types for operand 0 and 1/2 in cond_expr? > > Ah, that's a valid point... > >> I updated the patch following your suggestion. Note, in this way >> below range check on @2 should be redundant for EQ_EXPR case, but I >> didn't change that in this patch. >> >> if (int_fits_type_p (@2, from_type) >> && (types_match (c1_type, from_type) >> || (TYPE_PRECISION (c1_type) > TYPE_PRECISION (from_type) >> && (TYPE_UNSIGNED (from_type) >> || TYPE_SIGN (c1_type) == TYPE_SIGN (from_type)))) >> >> So which one should be preferred? > > I suppose it's better to use the same type and thus your version then > (-20161123). > That patch is ok. > > Note my worry here is usually that we already have conflicting foldings in this > area (moving conversions in/out), see fold_unary: > > /* If this was a conversion, and all we did was to move into > inside the COND_EXPR, bring it back out. But leave it if > it is a conversion from integer to integer and the > result precision is no wider than a word since such a > conversion is cheap and may be optimized away by combine, > while it couldn't if it were outside the COND_EXPR. Then return > so we don't get into an infinite recursion loop taking the > conversion out and then back in. */ > > if ((CONVERT_EXPR_CODE_P (code) > || code == NON_LVALUE_EXPR) > && TREE_CODE (tem) == COND_EXPR > && TREE_CODE (TREE_OPERAND (tem, 1)) == code > && TREE_CODE (TREE_OPERAND (tem, 2)) == code > && ! VOID_TYPE_P (TREE_OPERAND (tem, 1)) > && ! VOID_TYPE_P (TREE_OPERAND (tem, 2)) > && (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 1), 0)) > == TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 2), 0))) > && (! (INTEGRAL_TYPE_P (TREE_TYPE (tem)) > && (INTEGRAL_TYPE_P > (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 1), 0)))) > && TYPE_PRECISION (TREE_TYPE (tem)) <= BITS_PER_WORD) > || flag_syntax_only)) > tem = build1_loc (loc, code, type, > build3 (COND_EXPR, > TREE_TYPE (TREE_OPERAND > (TREE_OPERAND (tem, 1), 0)), > TREE_OPERAND (tem, 0), > TREE_OPERAND (TREE_OPERAND (tem, 1), 0), > TREE_OPERAND (TREE_OPERAND (tem, 2), > 0))); Yes, this is a potential issue. I will apply this version at the moment, we can always fall back to the other way if the conflict mentioned below becomes a real problem, especially for EQ_EXPR case. > > and fold_ternary has quite a bit of COND_EXPR folding as well. The new pattern is moved from fold_cond_expr_with_comparison which in turn is called from fold_ternary. There isn't much conflict between it and the rest cases in fold_ternary. IIUC, they target at different simplifications. I can add comment about conflict with fold_unary in the pattern, but would like to do that after fixing ICEs reported in PR78507 and PR78510. Thanks, bin > > Thanks, > Richard. > >> >> Thanks, >> bin >>> >>> Richard. >>> >>> >>>> Thanks, >>>> bin >>>>> >>>>>> Thanks, >>>>>> bin >>>>>> >>>>>> 2016-11-23 Bin Cheng <bin.cheng@arm.com> >>>>>> >>>>>> * match.pd: Refine type conversion in result expressions for below >>>>>> pattern: >>>>>> (cond (cmp (convert1? x) c1) (convert2? x) c2) -> (minmax (x c)).
Index: gcc/match.pd =================================================================== --- gcc/match.pd (revision 242751) +++ gcc/match.pd (working copy) @@ -2022,11 +2022,12 @@ } } (if (code == MAX_EXPR) - (convert (max @1 (convert:from_type @2))) + (convert (max @1 (convert @2))) (if (code == MIN_EXPR) - (convert (min @1 (convert:from_type @2))) + (convert (min @1 (convert @2))) (if (code == EQ_EXPR) - (cond (cmp @1 (convert:from_type @3)) (convert:from_type @3) @2))))))) + (convert (cond (cmp @1 (convert @3)) + (convert:from_type @3) (convert:from_type @2))))))))) (for cnd (cond vec_cond) /* A ? B : (A ? X : C) -> A ? B : C. */