Thanks Richard and Jakub for comments.
Ideally would like to make sure the imm operand will have exactly the same type
as operand 1.
But for uint8_t/uint16_t types, the INTERGER_CST will become the (const_int 3)
with int type before matching.
Thus, add the type check like that, as well as some negative test case like
fail to match .SAT_ADD (uint32_t, 3ull).. etc.
.SAT_ADD (uint8_t, (uint8_t)3u)
.SAT_ADD (uint16_t, (uint16_t)3u)
.SAT_ADD (uint32_t, 3u)
.SAT_ADD (uint64_t, 3ull)
Thanks again, good to know int_fits_type_p and let me have a try in v2.
Pan
-----Original Message-----
From: Jakub Jelinek <ja...@redhat.com>
Sent: Sunday, August 25, 2024 1:16 AM
To: Li, Pan2 <pan2...@intel.com>
Cc: gcc-patches@gcc.gnu.org; richard.guent...@gmail.com;
tamar.christ...@arm.com; juzhe.zh...@rivai.ai; kito.ch...@gmail.com;
jeffreya...@gmail.com; rdapp....@gmail.com
Subject: Re: [PATCH v1] Match: Add type check for .SAT_ADD imm operand
On Sat, Aug 24, 2024 at 07:33:06PM +0800, pan2...@intel.com wrote:
> From: Pan Li <pan2...@intel.com>
>
> This patch would like to add strict check for imm operand of .SAT_ADD
> matching. We have no type checking for imm operand in previous, which
> may result in unexpected IL to be catched by .SAT_ADD pattern.
>
> However, things may become more complicated due to the int promotion.
> This means any const_int without any suffix will be promoted to int
> before matching. For example as below.
>
> uint8_t a;
> uint8_t sum = .SAT_ADD (a, 12);
>
> The second operand will be (const_int 12) with int type when try to
> match .SAT_ADD. Thus, to support int8/int16 .SAT_ADD, only the
> int32 and int64 will be strictly checked.
>
> The below test suite are passed for this patch:
> * The rv64gcv fully regression test.
> * The x86 bootstrap test.
> * The x86 fully regression test.
>
> gcc/ChangeLog:
>
> * match.pd:
???
> * match.pd: Add strict type check for .SAT_ADD imm operand.
Usually you should say
* match.pd (pattern you change): What you change.
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -3190,7 +3190,16 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> (cond^ (ne (imagpart (IFN_ADD_OVERFLOW@2 @0 INTEGER_CST@1)) integer_zerop)
> integer_minus_onep (realpart @2))
> (if (INTEGRAL_TYPE_P (type) && TYPE_UNSIGNED (type)
> - && types_match (type, @0))))
> + && types_match (type, @0))
> + (with
> + {
> + unsigned precision = TYPE_PRECISION (type);
> + unsigned int_precision = HOST_BITS_PER_INT;
This has nothing to do with HOST_BITS_PER_INT.
The INTEGER_CST can have any type, not just int.
> + }
> + /* The const_int will perform int promotion, the const_int will have at
const_int (well, CONST_INT) is an RTL name, it is INTEGER_CST in GIMPLE.
Just one space after ,
> + least the int_precision. Thus, type less than int_precision will be
> + skipped the type match checking. */
But the whole comment doesn't make much sense to me, the INTEGER_CST won't
perform any int promotion.
> + (if (precision < int_precision || types_match (type, @1))))))
Why do you compare precision of type against anything?
You want to check that the INTEGER_CST@1 is representable in the type
(compatible with TREE_TYPE (@0)), because only then the caller can
fold_convert @1 to type without the value being altered.
So, IMHO best would be
(if (int_fits_type_p (@1, type))))))
Jakub
