On Fri, Sep 20, 2024 at 10:23 AM Kugan Vivekanandarajah
<kvivekana...@nvidia.com> wrote:
>
> Hi Richard,
>
> > On 17 Sep 2024, at 7:36 pm, Richard Biener <richard.guent...@gmail.com>
> > wrote:
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On Tue, Sep 17, 2024 at 10:31 AM Kugan Vivekanandarajah
> > <kvivekana...@nvidia.com> wrote:
> >>
> >> Hi Richard,
> >>
> >>> On 10 Sep 2024, at 9:33 pm, Richard Biener <richard.guent...@gmail.com>
> >>> wrote:
> >>>
> >>> External email: Use caution opening links or attachments
> >>>
> >>>
> >>> 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)))
> >>
> >> We also have to handle float type with convert:stype. Do you mean:
> >> (for cmp (ge gt)
> >> (simplify
> >> (cnd (cmp (convert?@0 @1) zerop) @2 (negate @2))
> >> (if (!HONOR_SIGNED_ZEROS (TREE_TYPE (@1))
> >> /* Support SEXT of @0 only. */
> >> && !TYPE_UNSIGNED (TREE_TYPE (@1))
> >> && element_precision (@1)
> >> <= element_precision (@0)
> >> && bitwise_equal_p (@1, @2))
> >> (with {
> >> tree stype = INTEGRAL_TYPE_P (type) ?
> >> signed_type_for (TREE_TYPE (@2)) : TREE_TYPE (@2);
> >> }
> >> /* 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 (TREE_TYPE (@2)))
> >> (absu (convert:stype @2))
> >> (abs (convert:stype @2)))))))
> >
> > I was thinking
> >
> > (for cmp (ge gt)
> > (simplify
> > (cnd (cmp (convert?@0 @1) zerop) @2 (negate @2))
> > (if (!HONOR_SIGNED_ZEROS (TREE_TYPE (@1))
> > /* Support SEXT of @0 only. */
> > && !TYPE_UNSIGNED (TREE_TYPE (@1))
> > && element_precision (@1)
> > <= element_precision (@0)
> > && bitwise_equal_p (@1, @2))
> > (if (TYPE_UNSIGNED (TREE_TYPE (@2)))
> > (with {
> > tree stype = signed_type_for (TREE_TYPE (@2));
> > }
> > (absu (convert:stype @2)))
> > (abs @2))))))
> >
> > if @2 is unsigned then 'type' is unsigned as well.
>
> Thanks. This works but this would require changing this:
> gcc.dg/tree-ssa/phi-opt-37.c: Check phiopt2 dump as shown in
> 0001-abshalffloat-simple.patch.
> If we do that optimisation Andrew suggested, it would be
> 0001-abshalffloat-handle_nop_convert.patch. This looks more complicated
> though.
Meh, this is getting so tedious. Can you split out the (type)A >=/> 0
? A : -A same as abs (A) part please?
We seem to have agreed on a version for that.
I don't understand what the issue is with the other, and I'm tired to
investigate myself so please
explain to me.
Richard.
> Bootstrapped and regression tested both versions.
>
> Thanks,
> Kugan
>
>
>
>
> >
> >>>
> >>> btw, you can write element_precision (@1) directly, the function handles
> >>> non-type arguments as well.
> >>
> >>
> >> Thanks,
> >> Kugan
> >>>
> >>> Richard.
> >>>
> >>>> Thanks,
> >>>> Kugan
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>> Andrew Pinski
>
>
