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))) btw, you can write element_precision (@1) directly, the function handles non-type arguments as well. Richard. > Thanks, > Kugan > > > > > Thanks, > > Andrew Pinski > >