Hi Richard,
> On 17 Sep 2024, at 7:36 pm, Richard Biener <richard.guent...@gmail.com> wrote:
>
> External email: Use caution opening links or attachments
>
>
> On Tue, Sep 17, 2024 at 10:31 AM Kugan Vivekanandarajah
> <kvivekana...@nvidia.com> wrote:
>>
>> Hi Richard,
>>
>>> On 10 Sep 2024, at 9:33 pm, Richard Biener <richard.guent...@gmail.com>
>>> wrote:
>>>
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 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)))
>>
>> We also have to handle float type with convert:stype. Do you mean:
>> (for cmp (ge gt)
>> (simplify
>> (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 (@1)
>> <= element_precision (@0)
>> && bitwise_equal_p (@1, @2))
>> (with {
>> tree stype = INTEGRAL_TYPE_P (type) ?
>> signed_type_for (TREE_TYPE (@2)) : TREE_TYPE (@2);
>> }
>> /* 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 (TREE_TYPE (@2)))
>> (absu (convert:stype @2))
>> (abs (convert:stype @2)))))))
>
> I was thinking
>
> (for cmp (ge gt)
> (simplify
> (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 (@1)
> <= element_precision (@0)
> && bitwise_equal_p (@1, @2))
> (if (TYPE_UNSIGNED (TREE_TYPE (@2)))
> (with {
> tree stype = signed_type_for (TREE_TYPE (@2));
> }
> (absu (convert:stype @2)))
> (abs @2))))))
>
> if @2 is unsigned then 'type' is unsigned as well.Thanks. This works but this would require changing this: gcc.dg/tree-ssa/phi-opt-37.c: Check phiopt2 dump as shown in 0001-abshalffloat-simple.patch. If we do that optimisation Andrew suggested, it would be 0001-abshalffloat-handle_nop_convert.patch. This looks more complicated though. Bootstrapped and regression tested both versions. Thanks, Kugan > >>> >>> btw, you can write element_precision (@1) directly, the function handles >>> non-type arguments as well. >> >> >> Thanks, >> Kugan >>> >>> Richard. >>> >>>> Thanks, >>>> Kugan >>>> >>>>> >>>>> Thanks, >>>>> Andrew Pinski
0001-abshalffloat-simple.patch
Description: 0001-abshalffloat-simple.patch
0001-abshalffloat-handle_nop_convert.patch
Description: 0001-abshalffloat-handle_nop_convert.patch
