On Thu, Nov 24, 2016 at 11:17 AM, Richard Biener <richard.guent...@gmail.com> wrote: > On Thu, Nov 24, 2016 at 11:11 AM, Bin.Cheng <amker.ch...@gmail.com> wrote: >> On Thu, Nov 24, 2016 at 8:57 AM, Richard Biener >> <richard.guent...@gmail.com> wrote: >>> On Wed, Nov 23, 2016 at 3:57 PM, Bin.Cheng <amker.ch...@gmail.com> wrote: >>>> On Wed, Nov 23, 2016 at 2:27 PM, Richard Biener >>>> <richard.guent...@gmail.com> wrote: >>>>> On Wed, Nov 23, 2016 at 2:54 PM, Bin Cheng <bin.ch...@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.ch...@arm.com> >>>>>> >>>>>> * match.pd: Refine type conversion in result expressions for >>>>>> below >>>>>> pattern: >>>>>> (cond (cmp (convert1? x) c1) (convert2? x) c2) -> (minmax (x c)).
