On Thu, Sep 5, 2024 at 3:19 AM Kugan Vivekanandarajah
<kvivekana...@nvidia.com> wrote:
>
> Thanks for the explanation.
>
>
> > On 2 Sep 2024, at 9:47 am, Andrew Pinski <pins...@gmail.com> wrote:
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On Sun, Sep 1, 2024 at 4:27 PM Kugan Vivekanandarajah
> > <kvivekana...@nvidia.com> wrote:
> >>
> >> Hi Andrew.
> >>
> >>> On 28 Aug 2024, at 2:23 pm, Andrew Pinski <pins...@gmail.com> wrote:
> >>>
> >>> External email: Use caution opening links or attachments
> >>>
> >>>
> >>> On Tue, Aug 27, 2024 at 8:54 PM Kugan Vivekanandarajah
> >>> <kvivekana...@nvidia.com> wrote:
> >>>>
> >>>> Hi Richard,
> >>>>
> >>>> Thanks for the reply.
> >>>>
> >>>>> On 27 Aug 2024, at 7:05 pm, Richard Biener <richard.guent...@gmail.com> 
> >>>>> wrote:
> >>>>>
> >>>>> External email: Use caution opening links or attachments
> >>>>>
> >>>>>
> >>>>> On Tue, Aug 27, 2024 at 8:23 AM Kugan Vivekanandarajah
> >>>>> <kvivekana...@nvidia.com> wrote:
> >>>>>>
> >>>>>> Hi Richard,
> >>>>>>
> >>>>>>> On 22 Aug 2024, at 10:34 pm, Richard Biener 
> >>>>>>> <richard.guent...@gmail.com> wrote:
> >>>>>>>
> >>>>>>> External email: Use caution opening links or attachments
> >>>>>>>
> >>>>>>>
> >>>>>>> On Wed, Aug 21, 2024 at 12:08 PM Kugan Vivekanandarajah
> >>>>>>> <kvivekana...@nvidia.com> wrote:
> >>>>>>>>
> >>>>>>>> Hi Richard,
> >>>>>>>>
> >>>>>>>>> On 20 Aug 2024, at 6:09 pm, Richard Biener 
> >>>>>>>>> <richard.guent...@gmail.com> wrote:
> >>>>>>>>>
> >>>>>>>>> External email: Use caution opening links or attachments
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Fri, Aug 9, 2024 at 2:39 AM Kugan Vivekanandarajah
> >>>>>>>>> <kvivekana...@nvidia.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Thanks for the comments.
> >>>>>>>>>>
> >>>>>>>>>>> On 2 Aug 2024, at 8:36 pm, Richard Biener 
> >>>>>>>>>>> <richard.guent...@gmail.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> External email: Use caution opening links or attachments
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Fri, Aug 2, 2024 at 11:20 AM Kugan Vivekanandarajah
> >>>>>>>>>>> <kvivekana...@nvidia.com> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> On 1 Aug 2024, at 10:46 pm, Richard Biener 
> >>>>>>>>>>>>> <richard.guent...@gmail.com> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> External email: Use caution opening links or attachments
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Thu, Aug 1, 2024 at 5:31 AM Kugan Vivekanandarajah
> >>>>>>>>>>>>> <kvivekana...@nvidia.com> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On Mon, Jul 29, 2024 at 10:11 AM Andrew Pinski 
> >>>>>>>>>>>>>> <pins...@gmail.com> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On Mon, Jul 29, 2024 at 12:57 AM Kugan Vivekanandarajah
> >>>>>>>>>>>>>>> <kugan...@gmail.com> wrote:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> On Thu, Jul 25, 2024 at 10:19 PM Richard Biener
> >>>>>>>>>>>>>>>> <richard.guent...@gmail.com> wrote:
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> On Thu, Jul 25, 2024 at 4:42 AM Kugan Vivekanandarajah
> >>>>>>>>>>>>>>>>> <kugan...@gmail.com> wrote:
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> On Tue, Jul 23, 2024 at 11:56 PM Richard Biener
> >>>>>>>>>>>>>>>>>> <richard.guent...@gmail.com> wrote:
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> On Tue, Jul 23, 2024 at 10:27 AM Kugan Vivekanandarajah
> >>>>>>>>>>>>>>>>>>> <kugan...@gmail.com> wrote:
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> On Tue, Jul 23, 2024 at 10:35 AM Andrew Pinski 
> >>>>>>>>>>>>>>>>>>>> <pins...@gmail.com> wrote:
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> On Mon, Jul 22, 2024 at 5:26 PM Kugan Vivekanandarajah
> >>>>>>>>>>>>>>>>>>>>> <kvivekana...@nvidia.com> wrote:
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> Revised based on the comment and moved it into 
> >>>>>>>>>>>>>>>>>>>>>> existing patterns as.
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> gcc/ChangeLog:
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> * match.pd: Extend A CMP 0 ? A : -A into (type)A CMP 0 
> >>>>>>>>>>>>>>>>>>>>>> ? A : -A.
> >>>>>>>>>>>>>>>>>>>>>> Extend A CMP 0 ? A : -A into (type) A CMP 0 ? A : -A.
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> gcc/testsuite/ChangeLog:
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> * gcc.dg/tree-ssa/absfloat16.c: New test.
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> The testcase needs to make sure it runs only for 
> >>>>>>>>>>>>>>>>>>>>> targets that support
> >>>>>>>>>>>>>>>>>>>>> float16 so like:
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> /* { dg-require-effective-target float16 } */
> >>>>>>>>>>>>>>>>>>>>> /* { dg-add-options float16 } */
> >>>>>>>>>>>>>>>>>>>> Added in the attached version.
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> + /* (type)A >=/> 0 ? A : -A    same as abs (A) */
> >>>>>>>>>>>>>>>>>>> (for cmp (ge gt)
> >>>>>>>>>>>>>>>>>>> (simplify
> >>>>>>>>>>>>>>>>>>> -   (cnd (cmp @0 zerop) @1 (negate @1))
> >>>>>>>>>>>>>>>>>>> -    (if (!HONOR_SIGNED_ZEROS (TREE_TYPE(@0))
> >>>>>>>>>>>>>>>>>>> -        && !TYPE_UNSIGNED (TREE_TYPE(@0))
> >>>>>>>>>>>>>>>>>>> -        && bitwise_equal_p (@0, @1))
> >>>>>>>>>>>>>>>>>>> +   (cnd (cmp (convert?@0 @1) zerop) @2 (negate @2))
> >>>>>>>>>>>>>>>>>>> +    (if (!HONOR_SIGNED_ZEROS (TREE_TYPE (@1))
> >>>>>>>>>>>>>>>>>>> +        && !TYPE_UNSIGNED (TREE_TYPE (@1))
> >>>>>>>>>>>>>>>>>>> +        && ((VECTOR_TYPE_P (type)
> >>>>>>>>>>>>>>>>>>> +             && tree_nop_conversion_p (TREE_TYPE (@0), 
> >>>>>>>>>>>>>>>>>>> TREE_TYPE (@1)))
> >>>>>>>>>>>>>>>>>>> +           || (!VECTOR_TYPE_P (type)
> >>>>>>>>>>>>>>>>>>> +               && (TYPE_PRECISION (TREE_TYPE (@1))
> >>>>>>>>>>>>>>>>>>> +                   <= TYPE_PRECISION (TREE_TYPE (@0)))))
> >>>>>>>>>>>>>>>>>>> +        && bitwise_equal_p (@1, @2))
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> I wonder about the bitwise_equal_p which tests @1 against 
> >>>>>>>>>>>>>>>>>>> @2 now
> >>>>>>>>>>>>>>>>>>> with the convert still applied to @1 - that looks odd.  
> >>>>>>>>>>>>>>>>>>> You are allowing
> >>>>>>>>>>>>>>>>>>> sign-changing conversions but doesn't that change ge/gt 
> >>>>>>>>>>>>>>>>>>> behavior?
> >>>>>>>>>>>>>>>>>>> Also why are sign/zero-extensions not OK for vector types?
> >>>>>>>>>>>>>>>>>> Thanks for the review.
> >>>>>>>>>>>>>>>>>> My main motivation here is for _Float16  as below.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> _Float16 absfloat16 (_Float16 x)
> >>>>>>>>>>>>>>>>>> {
> >>>>>>>>>>>>>>>>>> float _1;
> >>>>>>>>>>>>>>>>>> _Float16 _2;
> >>>>>>>>>>>>>>>>>> _Float16 _4;
> >>>>>>>>>>>>>>>>>> <bb 2> [local count: 1073741824]:
> >>>>>>>>>>>>>>>>>> _1 = (float) x_3(D);
> >>>>>>>>>>>>>>>>>> if (_1 < 0.0)
> >>>>>>>>>>>>>>>>>> goto <bb 3>; [41.00%]
> >>>>>>>>>>>>>>>>>> else
> >>>>>>>>>>>>>>>>>> goto <bb 4>; [59.00%]
> >>>>>>>>>>>>>>>>>> <bb 3> [local count: 440234144]:\
> >>>>>>>>>>>>>>>>>> _4 = -x_3(D);
> >>>>>>>>>>>>>>>>>> <bb 4> [local count: 1073741824]:
> >>>>>>>>>>>>>>>>>> # _2 = PHI <_4(3), x_3(D)(2)>
> >>>>>>>>>>>>>>>>>> return _2;
> >>>>>>>>>>>>>>>>>> }
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> This is why I added  bitwise_equal_p test of @1 against @2 
> >>>>>>>>>>>>>>>>>> with
> >>>>>>>>>>>>>>>>>> TYPE_PRECISION checks.
> >>>>>>>>>>>>>>>>>> I agree that I will have to check for sign-changing 
> >>>>>>>>>>>>>>>>>> conversions.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> Just to keep it simple, I disallowed vector types. I am 
> >>>>>>>>>>>>>>>>>> not sure if
> >>>>>>>>>>>>>>>>>> this would  hit vec types. I am happy to handle this if 
> >>>>>>>>>>>>>>>>>> that is
> >>>>>>>>>>>>>>>>>> needed.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> I think with __builtin_convertvector you should be able to 
> >>>>>>>>>>>>>>>>> construct
> >>>>>>>>>>>>>>>>> a testcase that does
> >>>>>>>>>>>>>>>> Thanks.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> For the pattern,
> >>>>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>>>> /* A >=/> 0 ? A : -A    same as abs (A) */
> >>>>>>>>>>>>>>>> (for cmp (ge gt)
> >>>>>>>>>>>>>>>> (simplify
> >>>>>>>>>>>>>>>> (cnd (cmp @0 zerop) @1 (negate @1))
> >>>>>>>>>>>>>>>> (if (!HONOR_SIGNED_ZEROS (TREE_TYPE(@0))
> >>>>>>>>>>>>>>>>   && !TYPE_UNSIGNED (TREE_TYPE(@0))
> >>>>>>>>>>>>>>>>   && bitwise_equal_p (@0, @1))
> >>>>>>>>>>>>>>>> (if (TYPE_UNSIGNED (type))
> >>>>>>>>>>>>>>>> (absu:type @0)
> >>>>>>>>>>>>>>>> (abs @0)))))
> >>>>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>>>> the vector type doesn't seem right. For example, if we have 
> >>>>>>>>>>>>>>>> a 4
> >>>>>>>>>>>>>>>> element vector with some negative and positive, I don't 
> >>>>>>>>>>>>>>>> think  it
> >>>>>>>>>>>>>>>> makes sense. Also, we dont seem to generate  (cmp @0 zerop). 
> >>>>>>>>>>>>>>>> Am I
> >>>>>>>>>>>>>>>> missing it completely?
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Looks like I missed adding some vector testcases anyways here 
> >>>>>>>>>>>>>>> is one
> >>>>>>>>>>>>>>> to get this, note it is C++ due to the C front-end not 
> >>>>>>>>>>>>>>> support `?:`
> >>>>>>>>>>>>>>> for vectors yet (there is a patch).
> >>>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>>> #define vect8 __attribute__((vector_size(8)))
> >>>>>>>>>>>>>>> vect8 int f(vect8 int a)
> >>>>>>>>>>>>>>> {
> >>>>>>>>>>>>>>> vect8 int na = -a;
> >>>>>>>>>>>>>>> return (a > 0) ? a : na;
> >>>>>>>>>>>>>>> }
> >>>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>>> At -O2 before forwprop1, we have:
> >>>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>>> vector(2) intD.9 a_2(D) = aD.2796;
> >>>>>>>>>>>>>>> vector(2) intD.9 naD.2799;
> >>>>>>>>>>>>>>> vector(2) <signed-boolean:32> _1;
> >>>>>>>>>>>>>>> vector(2) intD.9 _4;
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> na_3 = -a_2(D);
> >>>>>>>>>>>>>>> _1 = a_2(D) > { 0, 0 };
> >>>>>>>>>>>>>>> _4 = VEC_COND_EXPR <_1, a_2(D), na_3>;
> >>>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>>> And forwprop using match is able to do:
> >>>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>>> Applying pattern match.pd:6306, gimple-match-10.cc:19843
> >>>>>>>>>>>>>>> gimple_simplified to _4 = ABS_EXPR <a_2(D)>;
> >>>>>>>>>>>>>>> Removing dead stmt:_1 = a_2(D) > { 0, 0 };
> >>>>>>>>>>>>>>> Removing dead stmt:na_3 = -a_2(D);
> >>>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>>> (replace int with float and add  -fno-signed-zeros you can 
> >>>>>>>>>>>>>>> get the ABS also).
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Note comparisons with vector types always generate a vector 
> >>>>>>>>>>>>>>> boolean
> >>>>>>>>>>>>>>> type. So cond_expr will never show up with a vector 
> >>>>>>>>>>>>>>> comparison; only
> >>>>>>>>>>>>>>> vec_cond.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Thanks for the information. I have revised the patch by adding 
> >>>>>>>>>>>>>> test case for the vector type. I will look at removing the 
> >>>>>>>>>>>>>> VEC_COND_EXPR as a follow up.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> This is likely a major task so do not get you distracted - I 
> >>>>>>>>>>>>> just mentioned it.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> This is still rejected by the type checker.
> >>>>>>>>>>>>>> v2hi  absvect2 (v2hi x, int i) {
> >>>>>>>>>>>>>> v2hi neg = -x;
> >>>>>>>>>>>>>> v2si tmp = __builtin_convertvector (x, v2si);
> >>>>>>>>>>>>>> return (tmp > 0) ? x : neg;
> >>>>>>>>>>>>>> }
> >>>>>>>>>>>>>> as:
> >>>>>>>>>>>>>> error: incompatible vector types in conditional expression: 
> >>>>>>>>>>>>>> '__vector(2) int', 'v2hi' {aka '__vector(2) short int'} and 
> >>>>>>>>>>>>>> 'v2hi' {aka '__vector(2) short int'}
> >>>>>>>>>>>>>> 8 |     return (tmp > 0) ? x : neg;
> >>>>>>>>>>>>>> |            ~~~~~~~~~~^~~~~~~~~
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Also
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> -      (absu:type @0)
> >>>>>>>>>>>>>> -      (abs @0)))))
> >>>>>>>>>>>>>> +      (absu:type @1)
> >>>>>>>>>>>>>> +      (abs @1)))))
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Changing the @1 to @2  is resulting in failures.  Existing 
> >>>>>>>>>>>>>> tests like  the following would be optimized away in this case.
> >>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>> unsigned abs_with_convert0 (int x)
> >>>>>>>>>>>>>> {
> >>>>>>>>>>>>>> unsigned int y = x;
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> if (x < 0)
> >>>>>>>>>>>>>>  y = -y;
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> return y;
> >>>>>>>>>>>>>> }
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> How so - the TYPE_UNSIGNED tests should make the pattern not 
> >>>>>>>>>>>>> trigger here?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>> This follows the same intent as the original pattern.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Bootstrapped and regression tested on aarch64-linux-gnu. Is 
> >>>>>>>>>>>>>> this OK for trunk.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> -   (cnd (cmp @0 zerop) @1 (negate @1))
> >>>>>>>>>>>>> -    (if (!HONOR_SIGNED_ZEROS (TREE_TYPE(@0))
> >>>>>>>>>>>>> -        && !TYPE_UNSIGNED (TREE_TYPE(@0))
> >>>>>>>>>>>>> -        && bitwise_equal_p (@0, @1))
> >>>>>>>>>>>>> +   (cnd (cmp (convert?@0 @1) zerop) @2 (negate @2))
> >>>>>>>>>>>>> +    (if (!HONOR_SIGNED_ZEROS (TREE_TYPE (@1))
> >>>>>>>>>>>>> +        && !TYPE_UNSIGNED (TREE_TYPE (@1))
> >>>>>>>>>>>>> +        && !TYPE_UNSIGNED (TREE_TYPE (@0))
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I think the outer type could be allowed signed if ...
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +        && ((VECTOR_TYPE_P (TREE_TYPE (@0))
> >>>>>>>>>>>>> +             && VECTOR_TYPE_P (TREE_TYPE (@1))
> >>>>>>>>>>>>> +             && known_eq (TYPE_VECTOR_SUBPARTS (TREE_TYPE 
> >>>>>>>>>>>>> (@0)),
> >>>>>>>>>>>>> +                          TYPE_VECTOR_SUBPARTS (TREE_TYPE 
> >>>>>>>>>>>>> (@1)))
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> this is always true
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +             && element_precision (TREE_TYPE (@1))
> >>>>>>>>>>>>> +                 <= element_precision (TREE_TYPE (@0)))
> >>>>>>>>>>>>> +           || (!VECTOR_TYPE_P (TREE_TYPE (@0))
> >>>>>>>>>>>>> +               && (TYPE_PRECISION (TREE_TYPE (@1))
> >>>>>>>>>>>>> +                   <= TYPE_PRECISION (TREE_TYPE (@0)))))
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> ... you make the precision strictly larger.  With both unsigned 
> >>>>>>>>>>>>> the same
> >>>>>>>>>>>>> precision case should have been stripped anyway.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> You can use element_precision for both vector and non-vector so 
> >>>>>>>>>>>>> I think this
> >>>>>>>>>>>>> should simplify to just checking element_precision.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +      (absu:type @1)
> >>>>>>>>>>>>> +      (abs @1)))))
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I still think this needs to be @2 for type correctness.  @1 is 
> >>>>>>>>>>>>> only
> >>>>>>>>>>>>> bitwise equal,
> >>>>>>>>>>>>> it could have different sign.  I think we should drop 
> >>>>>>>>>>>>> bitwise_equal_p with the
> >>>>>>>>>>>>> convert now in and instead have a matching constraint.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Thanks, so this should translate to:
> >>>>>>>>>>>>
> >>>>>>>>>>>> /* (type)A >=/> 0 ? A : -A    same as abs (A) */
> >>>>>>>>>>>> (for cmp (ge gt)
> >>>>>>>>>>>> (simplify
> >>>>>>>>>>>> (cnd (cmp (convert?@0 @1) zerop) @2 (negate @2))
> >>>>>>>>>>>> (if (!HONOR_SIGNED_ZEROS (TREE_TYPE (@1))
> >>>>>>>>>>>>    && !TYPE_UNSIGNED (TREE_TYPE (@1))
> >>>>>>>>>>>
> >>>>>>>>>>> Oh, I mis-read this as unsigned, so this makes the conversions
> >>>>>>>>>>> to always sign-extend which means it's important to ensure
> >>>>>>>>>>> the type of @0 is also signed.
> >>>>>>>>>>>
> >>>>>>>>>>>>    && !TYPE_UNSIGNED (TREE_TYPE (@2))
> >>>>>>>>>>>>    && ((element_precision (TREE_TYPE (@1))
> >>>>>>>>>>>>           < element_precision (TREE_TYPE (@0))
> >>>>>>>>>>>>         || operand_equal_p (@1, @0)))
> >>>>>>>>>>>
> >>>>>>>>>>> which means <= might be better then.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>>    && bitwise_equal_p (@1, @2))
> >>>>>>>>>>>> (if (TYPE_UNSIGNED (type))
> >>>>>>>>>>>> (absu:type @2)
> >>>>>>>>>>>> (abs @2)))))
> >>>>>>>>>>>>
> >>>>>>>>>>>> However with this, I am seeing: Note that the @2 for these are 
> >>>>>>>>>>>> unsigned.
> >>>>>>>>>>>
> >>>>>>>>>>> I see, so we rely on @1 being the signed equivalent of @2 which 
> >>>>>>>>>>> might or
> >>>>>>>>>>> might not be signed.  I guess that indeed we want @2 here.
> >>>>>>>>>> Sorry I am not sure I understand this. When @2 is unsigned, 
> >>>>>>>>>> ABS_EXPR and ABSU_EXPR is not
> >>>>>>>>>> For example:
> >>>>>>>>>> With
> >>>>>>>>>> /* (type)A >=/> 0 ? A : -A    same as abs (A) */
> >>>>>>>>>> (for cmp (ge gt)
> >>>>>>>>>> (simplify
> >>>>>>>>>> (cnd (cmp (convert?@0 @1) zerop) @2 (negate @2))
> >>>>>>>>>> (if (!HONOR_SIGNED_ZEROS (TREE_TYPE (@1))
> >>>>>>>>>>     && !TYPE_UNSIGNED (TREE_TYPE (@0))
> >>>>>>>>>>     && !TYPE_UNSIGNED (TREE_TYPE (@1))
> >>>>>>>>>>     && element_precision (TREE_TYPE (@1))
> >>>>>>>>>>            <= element_precision (TREE_TYPE (@0))
> >>>>>>>>>>     && bitwise_equal_p (@1, @2))
> >>>>>>>>>> (if (TYPE_UNSIGNED (type))
> >>>>>>>>>>  (absu:type @2)
> >>>>>>>>>>  (abs @2)))))
> >>>>>>>>>>
> >>>>>>>>>> The test case below becomes:
> >>>>>>>>>>
> >>>>>>>>>> unsigned abs_with_convert0 (int x)
> >>>>>>>>>> {
> >>>>>>>>>> unsigned int y = x;
> >>>>>>>>>>
> >>>>>>>>>> if (x < 0)
> >>>>>>>>>>    y = -y;
> >>>>>>>>>>
> >>>>>>>>>> return y;
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> unsigned int abs_with_convert0 (int x)
> >>>>>>>>>> {
> >>>>>>>>>> unsigned int y;
> >>>>>>>>>>
> >>>>>>>>>> <bb 2> :
> >>>>>>>>>> y_3 = (unsigned int) x_2(D);
> >>>>>>>>>> if (x_2(D) < 0)
> >>>>>>>>>> goto <bb 3>; [INV]
> >>>>>>>>>> else
> >>>>>>>>>> goto <bb 4>; [INV]
> >>>>>>>>>>
> >>>>>>>>>> <bb 3> :
> >>>>>>>>>> y_4 = -y_3;
> >>>>>>>>>>
> >>>>>>>>>> <bb 4> :
> >>>>>>>>>> # y_1 = PHI <y_3(2), y_4(3)>
> >>>>>>>>>> return y_1;
> >>>>>>>>>>
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> To:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> unsigned int abs_with_convert0 (int x)
> >>>>>>>>>> {
> >>>>>>>>>> unsigned int y;
> >>>>>>>>>> unsigned int _5;
> >>>>>>>>>>
> >>>>>>>>>> <bb 2> :
> >>>>>>>>>> y_3 = (unsigned int) x_2(D);
> >>>>>>>>>> _5 = ABSU_EXPR <y_3>;
> >>>>>>>>>> return _5;
> >>>>>>>>>>
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> This is an invalid gimple. Are you suggesting that we should also 
> >>>>>>>>>> check if @2 is not UNSIGNED? If that is what you want, couple of 
> >>>>>>>>>> test cases in the test suite including phi-opt-37.c would fail.
> >>>>>>>>>
> >>>>>>>>> Trying to swap in the discussion after vacation ... iff @2 might be
> >>>>>>>>> unsigned then you'd need
> >>>>>>>>> a conversion to signed to make 'abs' make sense.
> >>>>>>>>>
> >>>>>>>>>> (cnd (cmp (convert?@0 @1) zerop) @2 (negate @2))
> >>>>>>>>>> (if (!HONOR_SIGNED_ZEROS (TREE_TYPE (@1))
> >>>>>>>>>>     && !TYPE_UNSIGNED (TREE_TYPE (@0))
> >>>>>>>>>>     && !TYPE_UNSIGNED (TREE_TYPE (@1))
> >>>>>>>>>
> >>>>>>>>> so this doesn't put any constraints on the signedness of @2
> >>>>>>>>>
> >>>>>>>>> What kind of extensions are valid on @1?  I think this warrants a
> >>>>>>>>> comment.  I think in the end the compare should be signed to
> >>>>>>>>> fit 'abs' semantics, but a zero-extension (unsigned @1) should be
> >>>>>>>>> OK?  So why constrain the sign of @1?
> >>>>>>>> I thing it is the conversion to signed (sign extend) that we want to 
> >>>>>>>> support, But for cases like:
> >>>>>>>> signed abs_with_convert1 (unsigned x)
> >>>>>>>> {
> >>>>>>>> int y = x;
> >>>>>>>>
> >>>>>>>> if (y < 0)
> >>>>>>>>     y = -x;
> >>>>>>>>
> >>>>>>>> return y;
> >>>>>>>> }
> >>>>>>>> @2 is  unsigned.
> >>>>>>>>
> >>>>>>>>>>     && element_precision (TREE_TYPE (@1))
> >>>>>>>>>>            <= element_precision (TREE_TYPE (@0))
> >>>>>>>>>>     && bitwise_equal_p (@1, @2))
> >>>>>>>>>> (if (TYPE_UNSIGNED (type))
> >>>>>>>>>>  (absu:type @2)
> >>>>>>>>>>  (abs @2)))))
> >>>>>>>>>
> >>>>>>>>> Of course then you'd want to convert @2 to signed?
> >>>>>>>>
> >>>>>>>> We have some test cases where @2 is unsigned where original pattern 
> >>>>>>>> triggers and will not with the new pattern. For example 
> >>>>>>>> gcc.dg/tree-ssa/phi-opt-37.c.
> >>>>>>>>
> >>>>>>>> Should we create a new pattern as:
> >>>>>>>> /* (type)A >=/> 0 ? A : -A    same as abs (A) */
> >>>>>>>> (for cmp (ge gt)
> >>>>>>>> (simplify
> >>>>>>>> (cnd (cmp (convert?@0 @1) zerop) @2 (negate @2))
> >>>>>>>> (if (!HONOR_SIGNED_ZEROS (TREE_TYPE (@1))
> >>>>>>>>      && !TYPE_UNSIGNED (TREE_TYPE (@1))
> >>>>>>>>      && !TYPE_UNSIGNED (TREE_TYPE (@2))
> >>>>>>>>      && element_precision (TREE_TYPE (@1))
> >>>>>>>>             <= element_precision (TREE_TYPE (@0))
> >>>>>>>>      && bitwise_equal_p (@1, @2))
> >>>>>>>>  (if (TYPE_UNSIGNED (type))
> >>>>>>>>   (abs @2)
> >>>>>>>>   (abs @2)))))
> >>>>>>>>
> >>>>>>>> And leave the old pattern as it is? Otherwise if we decide to use 
> >>>>>>>> @2, we have to XFAIL these cases unless I am missing spmething.
> >>>>>>>
> >>>>>>> No, I think we want a single pattern.  I meant you possibly need to do
> >>>>>>>
> >>>>>>> (with
> >>>>>>> { tree stype = signed_type_for (TREE_TYPE (@2)); }
> >>>>>>> (if (TYPE_UNSIGNED (type))
> >>>>>>> (absu (convert:stype @2))
> >>>>>>> (abs (convert:stype @2))))
> >>>>>>>
> >>>>>>> for when @2 is unsigned?
> >>>>>>
> >>>>>> Thanks. I have changed accordingly.
> >>>>>>
> >>>>>> I have also changed gcc.dg/tree-ssa/phi-opt-37.c to check phiopt2 as 
> >>>>>> in phiopt1 we don’t now match because of:
> >>>>>> phiopt match-simplify back:
> >>>>>> _5 = (signed int) x_2(D);
> >>>>>> _7 = ABSU_EXPR <_5>;
> >>>>>> result: _7
> >>>>>> rejected because early
> >>>>>
> >>>>> ?  I don't understand what you are saying.
> >>>>
> >>>> This is the dump from phiopt. Even though it is matching,
> >>>> /* Return TRUE if SEQ/OP pair should be allowed during early phiopt.
> >>>>  Currently this is to allow MIN/MAX and ABS/NEGATE and constants.  */
> >>>> static bool
> >>>> phiopt_early_allow (gimple_seq &seq, gimple_match_op &op) is rejecting 
> >>>> it, as the dump shows.
> >>>
> >>> So I looked into this slightly. I think the match patterns are greedy
> >>> when it comes to optional converts, in that match tries to match with
> >>> convert first and for integer types but bitwise_equal_p will handle a
> >>> nop_convert here.
> >>> So for scalar integer types, maybe skip `TYPE_PRECISION (TREE_TYPE
> >>> (@1)) == TYPE_PRECISION (TREE_TYPE (@0))` as that is handled via
> >>> bitwise_equal_p call and we don't want an extra convert being added by
> >>> the simplification
> >>
> >>
> >> phiopt_early_allow only allow single instruction sequences. But since we 
> >> now use @2 instead of @1, we will need the conversion. Sorry I don’t 
> >> understand the fix you want.
> >
> > The convert expression is already existing before the match.
> > ```
> >  y_3 = (int) x_2(D);
> >  if (y_3 < 0)
> >    goto <bb 3>; [INV]
> >  else
> >    goto <bb 4>; [INV]
> >
> >  <bb 3> :
> >  x_4 = -x_2(D);
> >
> >  <bb 4> :
> >  # x_1 = PHI <x_2(D)(2), x_4(3)>
> >  return x_1;
> > ```
> >
> > Adding a new convert via match and simplify is not useful and is the
> > cause of the issue as I mentioned
> >
> > So you should add:
> > ```
> >      (if (INTEGRAL_TYPE (type)
> >           && element_precision (TREE_TYPE (@1))
> >                  == element_precision (TREE_TYPE (@0)))
> >       (if (TYPE_UNSIGNED (type))
> > (absu:type @0)
> > (abs @0))
> > ```
> > So you reuse the convert expression that was there previously and the
> > testcase will just work.
>
> Here is the amended patch. Bootstrapped and regression tested on 
> aarch64-linux-gnu. Is this OK for trunk?

+      /* Handle nop convert first to prevent additional
+         convertion.  */
+      (if (INTEGRAL_TYPE_P (type)
+           && element_precision (TREE_TYPE (@1))
+              == element_precision (TREE_TYPE (@0)))
+       (if (TYPE_UNSIGNED (type))
+        (absu:type @0)
+        (abs @0))
+       (if (TYPE_UNSIGNED (type))
+        (if (TYPE_UNSIGNED (TREE_TYPE (@2)))
+        (absu (convert:stype @2))
+        (absu @2))
+        (if (TYPE_UNSIGNED (TREE_TYPE (@2)))
+        (abs (convert:stype @2))
+        (abs @2)))))))

I think this is overly complicated - an unnecessary conversion is elided by
match so you should be able to write just

     (if (TYPE_UNSIGNED (type))
      (absu (convert:stype @2))
      (abs (convert:stype @2)))

btw, you can write element_precision (@1) directly, the function handles
non-type arguments as well.

Richard.

> Thanks,
> Kugan
>
> >
> > Thanks,
> > Andrew Pinski
>
>

Reply via email to