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.
>
> + (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]>
> > > > >
