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.
> Bootstrapped and regression tested on aarch64-linux-gnu. Is this OK?
In both cases you do
- (cnd (cmp @0 zerop) @1 (negate @1))
- (if (!HONOR_SIGNED_ZEROS (TREE_TYPE(@0))
- && !TYPE_UNSIGNED (TREE_TYPE(@0))
...
+ (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 (TREE_TYPE (@1))
+ <= element_precision (TREE_TYPE (@0))
where formerly we require a signed comparison, a check that you drop
and one that IMO is important (I would guess for unsigned we never
match the pattern as compares against zero are simplified to eq/ne,
but still). I think that
unsigned a;
(int)a < 0 : -a : a
is valid to be matched to ABSU but is excluded by the requirement
of a sign-extension.
unsigned a;
(long)a < 0 : -a : a
isn't a ABSU though. That means
/* Support sign-change or SEXT of @0 only. */
&& (element_precision (TREE_TYPE (@1)) == element_precision (TREE_TYPE (@0))
|| (!TYPE_UNSIGNED (...)
&& ...))
would be better. That might also solve your problem with phi-opt-37.c?
Richard.
> Thanks,
> Kugan
>
>
>
> >
> > 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 <kvivekana...@nvidia.com>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 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
> >>>>>>>>>>>>>>>> <kvivekana...@nvidia.com>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Bootstrapped and regression test on aarch64-linux-gnu. Is
> >>>>>>>>>>>>>>>> this OK for trunk?
> >>>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>>> Kugan
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> ________________________________
> >>>>>>>>>>>>>>>> From: Andrew Pinski <pins...@gmail.com>
> >>>>>>>>>>>>>>>> Sent: Monday, 15 July 2024 5:30 AM
> >>>>>>>>>>>>>>>> To: Kugan Vivekanandarajah <kvivekana...@nvidia.com>
> >>>>>>>>>>>>>>>> Cc: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>;
> >>>>>>>>>>>>>>>> richard.guent...@gmail.com <richard.guent...@gmail.com>
> >>>>>>>>>>>>>>>> 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
> >>>>>>>>>>>>>>>> <kvivekana...@nvidia.com> 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
> >>>>>>>>>>>>>>>>> <kvivekana...@nvidia.com>
>
>
