On Wed, Aug 21, 2024 at 12:08 PM Kugan Vivekanandarajah
<[email protected]> wrote:
>
> Hi Richard,
>
> > On 20 Aug 2024, at 6:09 pm, Richard Biener <[email protected]>
> > wrote:
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On Fri, Aug 9, 2024 at 2:39 AM Kugan Vivekanandarajah
> > <[email protected]> wrote:
> >>
> >> Thanks for the comments.
> >>
> >>> On 2 Aug 2024, at 8:36 pm, Richard Biener <[email protected]>
> >>> wrote:
> >>>
> >>> External email: Use caution opening links or attachments
> >>>
> >>>
> >>> On Fri, Aug 2, 2024 at 11:20 AM Kugan Vivekanandarajah
> >>> <[email protected]> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> On 1 Aug 2024, at 10:46 pm, Richard Biener <[email protected]>
> >>>>> wrote:
> >>>>>
> >>>>> External email: Use caution opening links or attachments
> >>>>>
> >>>>>
> >>>>> On Thu, Aug 1, 2024 at 5:31 AM Kugan Vivekanandarajah
> >>>>> <[email protected]> wrote:
> >>>>>>
> >>>>>>
> >>>>>> On Mon, Jul 29, 2024 at 10:11 AM Andrew Pinski <[email protected]>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> On Mon, Jul 29, 2024 at 12:57 AM Kugan Vivekanandarajah
> >>>>>>> <[email protected]> wrote:
> >>>>>>>>
> >>>>>>>> On Thu, Jul 25, 2024 at 10:19 PM Richard Biener
> >>>>>>>> <[email protected]> wrote:
> >>>>>>>>>
> >>>>>>>>> On Thu, Jul 25, 2024 at 4:42 AM Kugan Vivekanandarajah
> >>>>>>>>> <[email protected]> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On Tue, Jul 23, 2024 at 11:56 PM Richard Biener
> >>>>>>>>>> <[email protected]> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On Tue, Jul 23, 2024 at 10:27 AM Kugan Vivekanandarajah
> >>>>>>>>>>> <[email protected]> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Tue, Jul 23, 2024 at 10:35 AM Andrew Pinski
> >>>>>>>>>>>> <[email protected]> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Mon, Jul 22, 2024 at 5:26 PM Kugan Vivekanandarajah
> >>>>>>>>>>>>> <[email protected]> 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?
Richard.
> Thanks,
> Kugan
> >
> >>
> >> Thanks,
> >> Kugan
> >>
> >>>
> >>>> Tests that now fail, but worked before (4 tests):
> >>>>
> >>>> gcc: gcc.dg/tree-ssa/phi-opt-36.c scan-tree-dump-not phiopt2 "if "
> >>>> gcc: gcc.dg/tree-ssa/phi-opt-36.c scan-tree-dump-times phiopt1 "if " 2
> >>>> gcc: gcc.dg/tree-ssa/phi-opt-37.c scan-tree-dump-not phiopt1 "if "gcc:
> >>>> gcc.dg/tree-ssa/phi-opt-37.c scan-tree-dump-times phiopt1 "ABSU_EXPR <" 2
> >>>>
> >>>>
> >>>> unsigned f2(int A)
> >>>> {
> >>>> unsigned t = A;
> >>>> // A >= 0? A : -A same as abs (A)
> >>>> if (A >= 0) return t;
> >>>> return -t;
> >>>> }
> >>>>
> >>>> unsigned abs_with_convert0 (int x)
> >>>> {
> >>>> unsigned int y = x;
> >>>>
> >>>> if (x < 0)
> >>>> y = -y;
> >>>>
> >>>> return y;
> >>>> }
> >>>> unsigned abs_with_convert1 (unsigned x)
> >>>> {
> >>>> int y = x;
> >>>>
> >>>> if (y < 0)
> >>>> x = -x;
> >>>>
> >>>> return x;
> >>>> }
> >>>>
> >>>> In the original pattern, we have check for !TYPE_UNSIGNED
> >>>> (TREE_TYPE(@0)) and (absu:type @0)
> >>>>
> >>>> Shouldnt that translate !TYPE_UNSIGNED (TREE_TYPE(@1)) and (absu:type
> >>>> @1) in the new pattern
> >>>>
> >>>> Thanks,
> >>>> Kugan
> >>>>
> >>>>>
> >>>>> Same comments apply to the 2nd pattern update.
> >>>>>
> >>>>> Richard.
> >>>>>
> >>>>>> Thanks,
> >>>>>> Kugan
> >>>>>>
> >>>>>> 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:
> >>>>>>
> >>>>>> * g++.dg/absvect.C: New test.
> >>>>>> * gcc.dg/tree-ssa/absfloat16.c: New test.
> >>>>>>
> >>>>>>
> >>>>>> signed-off-by: Kugan Vivekanandarajah <[email protected]>
> >>>>>>
> >>>>>>
> >>>>>> Note I think we should be able to merge VEC_COND_EXPR and COND_EXPR
> >>>>>> codes by instead looking whether the condition is vector or scalar.
> >>>>>> Fallout
> >>>>>> might be a bit too big, but it might be a thing to try.
> >>>>>>
> >>>>>> Richard.
> >>>>>>
> >>>>>>> Thanks,
> >>>>>>> Andrew Pinski
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> Kugan
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> + (absu:type @1)
> >>>>>>>>>>> + (abs @1)))))
> >>>>>>>>>>>
> >>>>>>>>>>> I think this should use @2 now.
> >>>>>>>>>> I will change this.
> >>>>>>>>>>
> >>>>>>>>>> Thanks,
> >>>>>>>>>> Kugan
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>> Thanks.
> >>>>>>>>>>>> Kugan
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> (like what is in gcc.dg/c11-floatn-3.c and others).
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Other than that it looks good but I can't approve it.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>> Andrew Pinski
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Signed-off-by: Kugan Vivekanandarajah <[email protected]>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Bootstrapped and regression test on aarch64-linux-gnu. Is this
> >>>>>>>>>>>>>> OK for trunk?
> >>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>> Kugan
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> ________________________________
> >>>>>>>>>>>>>> From: Andrew Pinski <[email protected]>
> >>>>>>>>>>>>>> Sent: Monday, 15 July 2024 5:30 AM
> >>>>>>>>>>>>>> To: Kugan Vivekanandarajah <[email protected]>
> >>>>>>>>>>>>>> Cc: [email protected] <[email protected]>;
> >>>>>>>>>>>>>> [email protected] <[email protected]>
> >>>>>>>>>>>>>> Subject: Re: [PATCH] MATCH: add abs support for half float
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> External email: Use caution opening links or attachments
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On Sun, Jul 14, 2024 at 1:12 AM Kugan Vivekanandarajah
> >>>>>>>>>>>>>> <[email protected]> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> This patch extends abs detection in matched for half float.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Bootstrapped and regression test on aarch64-linux-gnu. Is
> >>>>>>>>>>>>>>> this OK for trunk?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> This is basically this 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)))))
> >>>>>>>>>>>>>> ```
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> except extended to handle an optional convert. Why didn't you
> >>>>>>>>>>>>>> just
> >>>>>>>>>>>>>> extend the above pattern to handle the convert instead? Also I
> >>>>>>>>>>>>>> think
> >>>>>>>>>>>>>> you have an issue with unsigned types with the comparison.
> >>>>>>>>>>>>>> Also you should extend the -abs(A) pattern right below it in a
> >>>>>>>>>>>>>> similar fashion.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>> Andrew Pinski
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> gcc/ChangeLog:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> * match.pd: Add pattern to convert (type)A >=/> 0 ? A : -A
> >>>>>>>>>>>>>>> into abs (A) for half float.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> gcc/testsuite/ChangeLog:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> * gcc.dg/tree-ssa/absfloat16.c: New test.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Signed-off-by: Kugan Vivekanandarajah
> >>>>>>>>>>>>>>> <[email protected]>
>
>
