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.
Thanks,
Andrew Pinski
