On Wed, 27 Mar 2024, Jakub Jelinek wrote:
> Hi!
>
> The following patch attempts to fix the (view_convert (convert@0 @1))
> optimization. If TREE_TYPE (@0) is a _BitInt type with padding bits
> and @0 has the same precision as @1 and it has a different sign
> and _BitInt with padding bits are extended on the target (x86 doesn't,
> aarch64 doesn't, but arm plans to do that), then optimizing it to
> just (view_convert @1) is wrong, the padding bits wouldn't be what
> it should be.
> E.g. bitint-64.c test with -O2 has
> _5 = (unsigned _BitInt(5)) _4;
> _7 = (unsigned _BitInt(5)) e.0_1;
> _8 = _5 + _7;
> _9 = (_BitInt(5)) _8;
> _10 = VIEW_CONVERT_EXPR<unsigned char>(_9);
> and forwprop1 changes that to just
> _5 = (unsigned _BitInt(5)) _4;
> _7 = (unsigned _BitInt(5)) e.0_1;
> _8 = _5 + _7;
> _10 = VIEW_CONVERT_EXPR<unsigned char>(_8);
> The former makes the padding bits well defined (at least on arm in
> the future), while the latter has those bits zero extended.
So I think the existing rule, when extended to
|| (TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE
(@1))
&& TYPE_UNSIGNED (TREE_TYPE (@1))
assumes padding is extended according to the signedness. But the
original
&& (TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE
(@1))
rule has the padding undefined. I think that's inconsistent at least.
I'll note that we do not constrain 'type' so the V_C_E could
re-interpret the integer (with or without padding) as floating-point.
Given the previous
/* For integral conversions with the same precision or pointer
conversions use a NOP_EXPR instead. */
(simplify
(view_convert @0)
should match first we should only ever see non-integer/pointer here
or cases where the V_C_E looks at padding. IMO we should change
this to
&& ((TYPE_PRECISION (TREE_TYPE (@0)) >= TYPE_PRECISION (TREE_TYPE
(@1))
&& TYPE_SIGN (TREE_TYPE (@0)) == TYPE_SIGN (TREE_TYPE (@1))))
|| (TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE
(@1))
&& TYPE_UNSIGNED (TREE_TYPE (@1))))
? Having this inconsistent treatment of padding is bad.
In your case the precision is equal so this should fix it, right?
Not sure if it's worth adding a INTEGERAL_TYPE_P (type) case with
TYPE_PRECISION (type) == TYPE_PRECISION (TREE_TYPE (@1)), see the
previous pattern.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2024-03-26 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/114469
> * match.pd ((view_convert (convert@0 @1))): Don't optimize if
> TREE_TYPE (@0) is a BITINT_TYPE with padding bits which are supposed
> to be extended by the ABI.
>
> --- gcc/match.pd.jj 2024-03-15 11:04:24.672914747 +0100
> +++ gcc/match.pd 2024-03-26 15:49:44.177864509 +0100
> @@ -4699,13 +4699,38 @@ (define_operator_list SYNC_FETCH_AND_AND
> zero-extend while keeping the same size (for bool-to-char). */
> (simplify
> (view_convert (convert@0 @1))
> - (if ((INTEGRAL_TYPE_P (TREE_TYPE (@0)) || POINTER_TYPE_P (TREE_TYPE (@0)))
> - && (INTEGRAL_TYPE_P (TREE_TYPE (@1)) || POINTER_TYPE_P (TREE_TYPE
> (@1)))
> - && TYPE_SIZE (TREE_TYPE (@0)) == TYPE_SIZE (TREE_TYPE (@1))
> - && (TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE (@1))
> - || (TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE (@1))
> - && TYPE_UNSIGNED (TREE_TYPE (@1)))))
> - (view_convert @1)))
> + (with { tree type0 = TREE_TYPE (@0);
> + tree type1 = TREE_TYPE (@1);
> + bool ok = false;
> + if ((INTEGRAL_TYPE_P (type0) || POINTER_TYPE_P (type0))
> + && (INTEGRAL_TYPE_P (type1) || POINTER_TYPE_P (type1))
> + && TYPE_SIZE (type0) == TYPE_SIZE (type1))
> + {
> + if (TYPE_PRECISION (type0) == TYPE_PRECISION (type1))
> + {
> + if (TREE_CODE (type0) != BITINT_TYPE)
> + ok = true;
> + else
> + {
> + /* Avoid optimizing this if type0 is a _BitInt
> + type with padding bits which are supposed to be
> + extended. */
> + struct bitint_info info;
> + targetm.c.bitint_type_info (TYPE_PRECISION (type0),
> + &info);
> + if (!info.extended)
> + ok = true;
> + else
> + ok = (TYPE_PRECISION (type0)
> + == tree_to_uhwi (TYPE_SIZE (type0)));
> + }
> + }
> + else if (TYPE_PRECISION (type0) > TYPE_PRECISION (type1)
> + && TYPE_UNSIGNED (type1))
> + ok = true;
> + } }
> + (if (ok)
> + (view_convert @1))))
>
> /* Simplify a view-converted empty or single-element constructor. */
> (simplify
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
