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. > >> 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 From what I understand, we allow a signed comparison even now. We don’t allow unsigned comparison agains zero as earlier. We allow conversion of unsigned type to signed type before comparing. Am I missing something? > > unsigned a; > (int)a < 0 : -a : a > > is valid to be matched to ABSU but is excluded by the requirement > of a sign-extension. This is currently handled as : unsigned int abs1 (unsigned int x) { signed int _6; unsigned int _7; <bb 2> [local count: 1073741824]: _6 = (signed int) x_3(D); _7 = ABSU_EXPR <_6>; return _7; } > > unsigned a; > (long)a < 0 : -a : a > > isn't a ABSU though. That means Yes, this gets folded in .015.cfg as: Removing basic block 3 Merging blocks 2 and 4 Merging blocks 2 and 5 ;; 1 loops found ;; ;; Loop 0 ;; header 0, latch 1 ;; depth 0, outer -1 ;; nodes: 0 1 2 ;; 2 succs { 1 } unsigned int abs2 (unsigned int x) { unsigned int D.4427; <bb 2> : D.4427 = x; // predicted unlikely by early return (on trees) predictor. return D.4427; } Thanks, Kugan > > /* 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> >> >>