On Tue, Sep 17, 2024 at 10:31 AM Kugan Vivekanandarajah
<[email protected]> wrote:
>
> 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)))))))
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.
> >
> > btw, you can write element_precision (@1) directly, the function handles
> > non-type arguments as well.
>
>
> Thanks,
> Kugan
> >
> > Richard.
> >
> >> Thanks,
> >> Kugan
> >>
> >>>
> >>> Thanks,
> >>> Andrew Pinski
>
>
