Hi Richard,
> On 10 Sep 2024, at 9:33 pm, Richard Biener <[email protected]> wrote:
>
> External email: Use caution opening links or attachments
>
>
> On Thu, Sep 5, 2024 at 3:19 AM Kugan Vivekanandarajah
> <[email protected]> wrote:
>>
>> Thanks for the explanation.
>>
>>
>>> On 2 Sep 2024, at 9:47 am, Andrew Pinski <[email protected]> wrote:
>>>
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Sun, Sep 1, 2024 at 4:27 PM Kugan Vivekanandarajah
>>> <[email protected]> wrote:
>>>>
>>>> Hi Andrew.
>>>>
>>>>> On 28 Aug 2024, at 2:23 pm, Andrew Pinski <[email protected]> wrote:
>>>>>
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On Tue, Aug 27, 2024 at 8:54 PM Kugan Vivekanandarajah
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> Hi Richard,
>>>>>>
>>>>>> Thanks for the reply.
>>>>>>
>>>>>>> On 27 Aug 2024, at 7:05 pm, Richard Biener <[email protected]>
>>>>>>> wrote:
>>>>>>>
>>>>>>> External email: Use caution opening links or attachments
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Aug 27, 2024 at 8:23 AM Kugan Vivekanandarajah
>>>>>>> <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Hi Richard,
>>>>>>>>
>>>>>>>>> On 22 Aug 2024, at 10:34 pm, Richard Biener
>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>> External email: Use caution opening links or attachments
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Aug 21, 2024 at 12:08 PM Kugan Vivekanandarajah
>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Richard,
>>>>>>>>>>
>>>>>>>>>>> On 20 Aug 2024, at 6:09 pm, Richard Biener
>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> External email: Use caution opening links or attachments
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Aug 9, 2024 at 2:39 AM Kugan Vivekanandarajah
>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for the comments.
>>>>>>>>>>>>
>>>>>>>>>>>>> On 2 Aug 2024, at 8:36 pm, Richard Biener
>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> External email: Use caution opening links or attachments
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Fri, Aug 2, 2024 at 11:20 AM Kugan Vivekanandarajah
>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 1 Aug 2024, at 10:46 pm, Richard Biener
>>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> External email: Use caution opening links or attachments
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Thu, Aug 1, 2024 at 5:31 AM Kugan Vivekanandarajah
>>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Mon, Jul 29, 2024 at 10:11 AM Andrew Pinski
>>>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Mon, Jul 29, 2024 at 12:57 AM Kugan Vivekanandarajah
>>>>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Thu, Jul 25, 2024 at 10:19 PM Richard Biener
>>>>>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On Thu, Jul 25, 2024 at 4:42 AM Kugan Vivekanandarajah
>>>>>>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On Tue, Jul 23, 2024 at 11:56 PM Richard Biener
>>>>>>>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 23, 2024 at 10:27 AM Kugan Vivekanandarajah
>>>>>>>>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 23, 2024 at 10:35 AM Andrew Pinski
>>>>>>>>>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> On Mon, Jul 22, 2024 at 5:26 PM Kugan Vivekanandarajah
>>>>>>>>>>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Revised based on the comment and moved it into
>>>>>>>>>>>>>>>>>>>>>>>> existing patterns as.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> gcc/ChangeLog:
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> * match.pd: Extend A CMP 0 ? A : -A into (type)A CMP 0
>>>>>>>>>>>>>>>>>>>>>>>> ? A : -A.
>>>>>>>>>>>>>>>>>>>>>>>> Extend A CMP 0 ? A : -A into (type) A CMP 0 ? A : -A.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> * gcc.dg/tree-ssa/absfloat16.c: New test.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> The testcase needs to make sure it runs only for
>>>>>>>>>>>>>>>>>>>>>>> targets that support
>>>>>>>>>>>>>>>>>>>>>>> float16 so like:
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> /* { dg-require-effective-target float16 } */
>>>>>>>>>>>>>>>>>>>>>>> /* { dg-add-options float16 } */
>>>>>>>>>>>>>>>>>>>>>> Added in the attached version.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> + /* (type)A >=/> 0 ? A : -A same as abs (A) */
>>>>>>>>>>>>>>>>>>>>> (for cmp (ge gt)
>>>>>>>>>>>>>>>>>>>>> (simplify
>>>>>>>>>>>>>>>>>>>>> - (cnd (cmp @0 zerop) @1 (negate @1))
>>>>>>>>>>>>>>>>>>>>> - (if (!HONOR_SIGNED_ZEROS (TREE_TYPE(@0))
>>>>>>>>>>>>>>>>>>>>> - && !TYPE_UNSIGNED (TREE_TYPE(@0))
>>>>>>>>>>>>>>>>>>>>> - && bitwise_equal_p (@0, @1))
>>>>>>>>>>>>>>>>>>>>> + (cnd (cmp (convert?@0 @1) zerop) @2 (negate @2))
>>>>>>>>>>>>>>>>>>>>> + (if (!HONOR_SIGNED_ZEROS (TREE_TYPE (@1))
>>>>>>>>>>>>>>>>>>>>> + && !TYPE_UNSIGNED (TREE_TYPE (@1))
>>>>>>>>>>>>>>>>>>>>> + && ((VECTOR_TYPE_P (type)
>>>>>>>>>>>>>>>>>>>>> + && tree_nop_conversion_p (TREE_TYPE (@0),
>>>>>>>>>>>>>>>>>>>>> TREE_TYPE (@1)))
>>>>>>>>>>>>>>>>>>>>> + || (!VECTOR_TYPE_P (type)
>>>>>>>>>>>>>>>>>>>>> + && (TYPE_PRECISION (TREE_TYPE (@1))
>>>>>>>>>>>>>>>>>>>>> + <= TYPE_PRECISION (TREE_TYPE (@0)))))
>>>>>>>>>>>>>>>>>>>>> + && bitwise_equal_p (@1, @2))
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> I wonder about the bitwise_equal_p which tests @1 against
>>>>>>>>>>>>>>>>>>>>> @2 now
>>>>>>>>>>>>>>>>>>>>> with the convert still applied to @1 - that looks odd.
>>>>>>>>>>>>>>>>>>>>> You are allowing
>>>>>>>>>>>>>>>>>>>>> sign-changing conversions but doesn't that change ge/gt
>>>>>>>>>>>>>>>>>>>>> behavior?
>>>>>>>>>>>>>>>>>>>>> Also why are sign/zero-extensions not OK for vector types?
>>>>>>>>>>>>>>>>>>>> Thanks for the review.
>>>>>>>>>>>>>>>>>>>> My main motivation here is for _Float16 as below.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> _Float16 absfloat16 (_Float16 x)
>>>>>>>>>>>>>>>>>>>> {
>>>>>>>>>>>>>>>>>>>> float _1;
>>>>>>>>>>>>>>>>>>>> _Float16 _2;
>>>>>>>>>>>>>>>>>>>> _Float16 _4;
>>>>>>>>>>>>>>>>>>>> <bb 2> [local count: 1073741824]:
>>>>>>>>>>>>>>>>>>>> _1 = (float) x_3(D);
>>>>>>>>>>>>>>>>>>>> if (_1 < 0.0)
>>>>>>>>>>>>>>>>>>>> goto <bb 3>; [41.00%]
>>>>>>>>>>>>>>>>>>>> else
>>>>>>>>>>>>>>>>>>>> goto <bb 4>; [59.00%]
>>>>>>>>>>>>>>>>>>>> <bb 3> [local count: 440234144]:\
>>>>>>>>>>>>>>>>>>>> _4 = -x_3(D);
>>>>>>>>>>>>>>>>>>>> <bb 4> [local count: 1073741824]:
>>>>>>>>>>>>>>>>>>>> # _2 = PHI <_4(3), x_3(D)(2)>
>>>>>>>>>>>>>>>>>>>> return _2;
>>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> This is why I added bitwise_equal_p test of @1 against @2
>>>>>>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>>>>>>> TYPE_PRECISION checks.
>>>>>>>>>>>>>>>>>>>> I agree that I will have to check for sign-changing
>>>>>>>>>>>>>>>>>>>> conversions.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Just to keep it simple, I disallowed vector types. I am
>>>>>>>>>>>>>>>>>>>> not sure if
>>>>>>>>>>>>>>>>>>>> this would hit vec types. I am happy to handle this if
>>>>>>>>>>>>>>>>>>>> that is
>>>>>>>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 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)))))))
>
> btw, you can write element_precision (@1) directly, the function handles
> non-type arguments as well.
Thanks,
Kugan
>
> Richard.
>
>> Thanks,
>> Kugan
>>
>>>
>>> Thanks,
>>> Andrew Pinski
