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?
Thanks,
Kugan
>
> Thanks,
> Andrew Pinski
0001-abshalffloat-v6.patch
Description: 0001-abshalffloat-v6.patch
