diff mbox series

[v1] Match: Add type check for .SAT_ADD imm operand

Message ID 20240824113306.1364233-1-pan2.li@intel.com
State New
Headers show
Series [v1] Match: Add type check for .SAT_ADD imm operand | expand

Commit Message

Li, Pan2 Aug. 24, 2024, 11:33 a.m. UTC
From: Pan Li <pan2.li@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.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/sat_u_add_imm-11.c: Adjust test case for imm.
	* gcc.target/riscv/sat_u_add_imm-12.c: Ditto.
	* gcc.target/riscv/sat_u_add_imm-15.c: Ditto.
	* gcc.target/riscv/sat_u_add_imm-16.c: Ditto.
	* gcc.target/riscv/sat_u_add_imm_type_check-1.c: New test.
	* gcc.target/riscv/sat_u_add_imm_type_check-2.c: New test.
	* gcc.target/riscv/sat_u_add_imm_type_check-3.c: New test.
	* gcc.target/riscv/sat_u_add_imm_type_check-4.c: New test.
	* gcc.target/riscv/sat_u_add_imm_type_check-5.c: New test.
	* gcc.target/riscv/sat_u_add_imm_type_check-6.c: New test.

Signed-off-by: Pan Li <pan2.li@intel.com>
---
 gcc/match.pd                                          | 11 ++++++++++-
 gcc/testsuite/gcc.target/riscv/sat_u_add_imm-11.c     |  4 ++--
 gcc/testsuite/gcc.target/riscv/sat_u_add_imm-12.c     |  4 ++--
 gcc/testsuite/gcc.target/riscv/sat_u_add_imm-15.c     |  4 ++--
 gcc/testsuite/gcc.target/riscv/sat_u_add_imm-16.c     |  4 ++--
 .../gcc.target/riscv/sat_u_add_imm_type_check-1.c     |  9 +++++++++
 .../gcc.target/riscv/sat_u_add_imm_type_check-2.c     |  9 +++++++++
 .../gcc.target/riscv/sat_u_add_imm_type_check-3.c     |  9 +++++++++
 .../gcc.target/riscv/sat_u_add_imm_type_check-4.c     |  9 +++++++++
 .../gcc.target/riscv/sat_u_add_imm_type_check-5.c     |  9 +++++++++
 .../gcc.target/riscv/sat_u_add_imm_type_check-6.c     |  9 +++++++++
 11 files changed, 72 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-2.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-3.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-4.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-5.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-6.c

Comments

Richard Biener Aug. 24, 2024, 1:28 p.m. UTC | #1
> Am 24.08.2024 um 13:33 schrieb pan2.li@intel.com:
> 
> From: Pan Li <pan2.li@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.
> 
> gcc/testsuite/ChangeLog:
> 
>    * gcc.target/riscv/sat_u_add_imm-11.c: Adjust test case for imm.
>    * gcc.target/riscv/sat_u_add_imm-12.c: Ditto.
>    * gcc.target/riscv/sat_u_add_imm-15.c: Ditto.
>    * gcc.target/riscv/sat_u_add_imm-16.c: Ditto.
>    * gcc.target/riscv/sat_u_add_imm_type_check-1.c: New test.
>    * gcc.target/riscv/sat_u_add_imm_type_check-2.c: New test.
>    * gcc.target/riscv/sat_u_add_imm_type_check-3.c: New test.
>    * gcc.target/riscv/sat_u_add_imm_type_check-4.c: New test.
>    * gcc.target/riscv/sat_u_add_imm_type_check-5.c: New test.
>    * gcc.target/riscv/sat_u_add_imm_type_check-6.c: New test.
> 
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
> gcc/match.pd                                          | 11 ++++++++++-
> gcc/testsuite/gcc.target/riscv/sat_u_add_imm-11.c     |  4 ++--
> gcc/testsuite/gcc.target/riscv/sat_u_add_imm-12.c     |  4 ++--
> gcc/testsuite/gcc.target/riscv/sat_u_add_imm-15.c     |  4 ++--
> gcc/testsuite/gcc.target/riscv/sat_u_add_imm-16.c     |  4 ++--
> .../gcc.target/riscv/sat_u_add_imm_type_check-1.c     |  9 +++++++++
> .../gcc.target/riscv/sat_u_add_imm_type_check-2.c     |  9 +++++++++
> .../gcc.target/riscv/sat_u_add_imm_type_check-3.c     |  9 +++++++++
> .../gcc.target/riscv/sat_u_add_imm_type_check-4.c     |  9 +++++++++
> .../gcc.target/riscv/sat_u_add_imm_type_check-5.c     |  9 +++++++++
> .../gcc.target/riscv/sat_u_add_imm_type_check-6.c     |  9 +++++++++
> 11 files changed, 72 insertions(+), 9 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-1.c
> create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-2.c
> create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-3.c
> create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-4.c
> create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-5.c
> create mode 100644 gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-6.c
> 
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 65a3aae2243..f695790629e 100644
> --- 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;
> +    }
> +    /* The const_int will perform int promotion,  the const_int will have at
> +       least the int_precision.  Thus, type less than int_precision will be
> +       skipped the type match checking.  */
> +    (if (precision < int_precision || types_match (type, @1))))))

I don’t think this is an improvement.  What are the constraints on the type so that the saturating operation is correcly matched?
If you let through wrong types here you will still have to deal with that in the consumers.
Note there’s int_fits_type which might come handy- it’s just not clear which constants form a valid saturating operation.

Richard 

> 
> /* Unsigned saturation sub, case 1 (branch with gt):
>    SAT_U_SUB = X > Y ? X - Y : 0  */
> diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-11.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-11.c
> index 43f34b5f3c9..a246e9b1857 100644
> --- a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-11.c
> +++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-11.c
> @@ -5,7 +5,7 @@
> #include "sat_arith.h"
> 
> /*
> -** sat_u_add_imm7_uint32_t_fmt_3:
> +** sat_u_add_imm7u_uint32_t_fmt_3:
> ** slli\s+[atx][0-9]+,\s*a0,\s*32
> ** srli\s+[atx][0-9]+,\s*[atx][0-9]+,\s*32
> ** addi\s+[atx][0-9]+,\s*a0,\s*7
> @@ -17,6 +17,6 @@
> ** sext.w\s+a0,\s*a0
> ** ret
> */
> -DEF_SAT_U_ADD_IMM_FMT_3(uint32_t, 7)
> +DEF_SAT_U_ADD_IMM_FMT_3(uint32_t, 7u)
> 
> /* { dg-final { scan-rtl-dump-times ".SAT_ADD " 2 "expand" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-12.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-12.c
> index 561c127f5fa..143f14c3af0 100644
> --- a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-12.c
> +++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-12.c
> @@ -5,13 +5,13 @@
> #include "sat_arith.h"
> 
> /*
> -** sat_u_add_imm8_uint64_t_fmt_3:
> +** sat_u_add_imm8ull_uint64_t_fmt_3:
> ** addi\s+[atx][0-9]+,\s*a0,\s*8
> ** sltu\s+[atx][0-9]+,\s*[atx][0-9]+,\s*[atx][0-9]+
> ** neg\s+[atx][0-9]+,\s*[atx][0-9]+
> ** or\s+a0,\s*[atx][0-9]+,\s*[atx][0-9]+
> ** ret
> */
> -DEF_SAT_U_ADD_IMM_FMT_3(uint64_t, 8)
> +DEF_SAT_U_ADD_IMM_FMT_3(uint64_t, 8ull)
> 
> /* { dg-final { scan-rtl-dump-times ".SAT_ADD " 2 "expand" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-15.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-15.c
> index eeea574bba2..995a02bffff 100644
> --- a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-15.c
> +++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-15.c
> @@ -5,7 +5,7 @@
> #include "sat_arith.h"
> 
> /*
> -** sat_u_add_imm7_uint32_t_fmt_4:
> +** sat_u_add_imm7u_uint32_t_fmt_4:
> ** slli\s+[atx][0-9]+,\s*a0,\s*32
> ** srli\s+[atx][0-9]+,\s*[atx][0-9]+,\s*32
> ** addi\s+[atx][0-9]+,\s*a0,\s*7
> @@ -17,6 +17,6 @@
> ** sext.w\s+a0,\s*a0
> ** ret
> */
> -DEF_SAT_U_ADD_IMM_FMT_4(uint32_t, 7)
> +DEF_SAT_U_ADD_IMM_FMT_4(uint32_t, 7u)
> 
> /* { dg-final { scan-rtl-dump-times ".SAT_ADD " 2 "expand" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-16.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-16.c
> index 307b81589ee..65e5a4a99bb 100644
> --- a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-16.c
> +++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-16.c
> @@ -5,13 +5,13 @@
> #include "sat_arith.h"
> 
> /*
> -** sat_u_add_imm8_uint64_t_fmt_4:
> +** sat_u_add_imm8ull_uint64_t_fmt_4:
> ** addi\s+[atx][0-9]+,\s*a0,\s*8
> ** sltu\s+[atx][0-9]+,\s*[atx][0-9]+,\s*[atx][0-9]+
> ** neg\s+[atx][0-9]+,\s*[atx][0-9]+
> ** or\s+a0,\s*[atx][0-9]+,\s*[atx][0-9]+
> ** ret
> */
> -DEF_SAT_U_ADD_IMM_FMT_4(uint64_t, 8)
> +DEF_SAT_U_ADD_IMM_FMT_4(uint64_t, 8ull)
> 
> /* { dg-final { scan-rtl-dump-times ".SAT_ADD " 2 "expand" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-1.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-1.c
> new file mode 100644
> index 00000000000..81117a3e6de
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-1.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
> +
> +#include "sat_arith.h"
> +
> +DEF_SAT_U_ADD_IMM_FMT_3(uint8_t, 9)
> +DEF_SAT_U_ADD_IMM_FMT_4(uint8_t, 9)
> +
> +/* { dg-final { scan-rtl-dump-times ".SAT_ADD " 4 "expand" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-2.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-2.c
> new file mode 100644
> index 00000000000..d5de0960a96
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-2.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
> +
> +#include "sat_arith.h"
> +
> +DEF_SAT_U_ADD_IMM_FMT_3(uint16_t, 91)
> +DEF_SAT_U_ADD_IMM_FMT_4(uint16_t, 91)
> +
> +/* { dg-final { scan-rtl-dump-times ".SAT_ADD " 4 "expand" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-3.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-3.c
> new file mode 100644
> index 00000000000..a3391ca460a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-3.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
> +
> +#include "sat_arith.h"
> +
> +DEF_SAT_U_ADD_IMM_FMT_3(uint32_t, 191u)
> +DEF_SAT_U_ADD_IMM_FMT_4(uint32_t, 191u)
> +
> +/* { dg-final { scan-rtl-dump-times ".SAT_ADD " 4 "expand" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-4.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-4.c
> new file mode 100644
> index 00000000000..bc12e0e346f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-4.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
> +
> +#include "sat_arith.h"
> +
> +DEF_SAT_U_ADD_IMM_FMT_3(uint64_t, 191)
> +DEF_SAT_U_ADD_IMM_FMT_4(uint64_t, 191)
> +
> +/* { dg-final { scan-rtl-dump-not ".SAT_ADD " "expand" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-5.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-5.c
> new file mode 100644
> index 00000000000..9b92d9624b5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-5.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
> +
> +#include "sat_arith.h"
> +
> +DEF_SAT_U_ADD_IMM_FMT_3(uint64_t, 191ull)
> +DEF_SAT_U_ADD_IMM_FMT_4(uint64_t, 191ull)
> +
> +/* { dg-final { scan-rtl-dump-times ".SAT_ADD " 4 "expand" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-6.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-6.c
> new file mode 100644
> index 00000000000..a64f417bda3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-6.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
> +
> +#include "sat_arith.h"
> +
> +DEF_SAT_U_ADD_IMM_FMT_3(uint32_t, 191ull)
> +DEF_SAT_U_ADD_IMM_FMT_4(uint32_t, 191ull)
> +
> +/* { dg-final { scan-rtl-dump-not ".SAT_ADD " "expand" } } */
> --
> 2.43.0
>
Jakub Jelinek Aug. 24, 2024, 5:16 p.m. UTC | #2
On Sat, Aug 24, 2024 at 07:33:06PM +0800, pan2.li@intel.com wrote:
> From: Pan Li <pan2.li@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
Li, Pan2 Aug. 25, 2024, 2:07 a.m. UTC | #3
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 <jakub@redhat.com> 
Sent: Sunday, August 25, 2024 1:16 AM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; richard.guenther@gmail.com; Tamar.Christina@arm.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; jeffreyalaw@gmail.com; rdapp.gcc@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.li@intel.com wrote:
> From: Pan Li <pan2.li@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
diff mbox series

Patch

diff --git a/gcc/match.pd b/gcc/match.pd
index 65a3aae2243..f695790629e 100644
--- 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;
+    }
+    /* The const_int will perform int promotion,  the const_int will have at
+       least the int_precision.  Thus, type less than int_precision will be
+       skipped the type match checking.  */
+    (if (precision < int_precision || types_match (type, @1))))))
 
 /* Unsigned saturation sub, case 1 (branch with gt):
    SAT_U_SUB = X > Y ? X - Y : 0  */
diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-11.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-11.c
index 43f34b5f3c9..a246e9b1857 100644
--- a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-11.c
+++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-11.c
@@ -5,7 +5,7 @@ 
 #include "sat_arith.h"
 
 /*
-** sat_u_add_imm7_uint32_t_fmt_3:
+** sat_u_add_imm7u_uint32_t_fmt_3:
 ** slli\s+[atx][0-9]+,\s*a0,\s*32
 ** srli\s+[atx][0-9]+,\s*[atx][0-9]+,\s*32
 ** addi\s+[atx][0-9]+,\s*a0,\s*7
@@ -17,6 +17,6 @@ 
 ** sext.w\s+a0,\s*a0
 ** ret
 */
-DEF_SAT_U_ADD_IMM_FMT_3(uint32_t, 7)
+DEF_SAT_U_ADD_IMM_FMT_3(uint32_t, 7u)
 
 /* { dg-final { scan-rtl-dump-times ".SAT_ADD " 2 "expand" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-12.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-12.c
index 561c127f5fa..143f14c3af0 100644
--- a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-12.c
+++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-12.c
@@ -5,13 +5,13 @@ 
 #include "sat_arith.h"
 
 /*
-** sat_u_add_imm8_uint64_t_fmt_3:
+** sat_u_add_imm8ull_uint64_t_fmt_3:
 ** addi\s+[atx][0-9]+,\s*a0,\s*8
 ** sltu\s+[atx][0-9]+,\s*[atx][0-9]+,\s*[atx][0-9]+
 ** neg\s+[atx][0-9]+,\s*[atx][0-9]+
 ** or\s+a0,\s*[atx][0-9]+,\s*[atx][0-9]+
 ** ret
 */
-DEF_SAT_U_ADD_IMM_FMT_3(uint64_t, 8)
+DEF_SAT_U_ADD_IMM_FMT_3(uint64_t, 8ull)
 
 /* { dg-final { scan-rtl-dump-times ".SAT_ADD " 2 "expand" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-15.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-15.c
index eeea574bba2..995a02bffff 100644
--- a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-15.c
+++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-15.c
@@ -5,7 +5,7 @@ 
 #include "sat_arith.h"
 
 /*
-** sat_u_add_imm7_uint32_t_fmt_4:
+** sat_u_add_imm7u_uint32_t_fmt_4:
 ** slli\s+[atx][0-9]+,\s*a0,\s*32
 ** srli\s+[atx][0-9]+,\s*[atx][0-9]+,\s*32
 ** addi\s+[atx][0-9]+,\s*a0,\s*7
@@ -17,6 +17,6 @@ 
 ** sext.w\s+a0,\s*a0
 ** ret
 */
-DEF_SAT_U_ADD_IMM_FMT_4(uint32_t, 7)
+DEF_SAT_U_ADD_IMM_FMT_4(uint32_t, 7u)
 
 /* { dg-final { scan-rtl-dump-times ".SAT_ADD " 2 "expand" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-16.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-16.c
index 307b81589ee..65e5a4a99bb 100644
--- a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-16.c
+++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm-16.c
@@ -5,13 +5,13 @@ 
 #include "sat_arith.h"
 
 /*
-** sat_u_add_imm8_uint64_t_fmt_4:
+** sat_u_add_imm8ull_uint64_t_fmt_4:
 ** addi\s+[atx][0-9]+,\s*a0,\s*8
 ** sltu\s+[atx][0-9]+,\s*[atx][0-9]+,\s*[atx][0-9]+
 ** neg\s+[atx][0-9]+,\s*[atx][0-9]+
 ** or\s+a0,\s*[atx][0-9]+,\s*[atx][0-9]+
 ** ret
 */
-DEF_SAT_U_ADD_IMM_FMT_4(uint64_t, 8)
+DEF_SAT_U_ADD_IMM_FMT_4(uint64_t, 8ull)
 
 /* { dg-final { scan-rtl-dump-times ".SAT_ADD " 2 "expand" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-1.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-1.c
new file mode 100644
index 00000000000..81117a3e6de
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-1.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
+
+#include "sat_arith.h"
+
+DEF_SAT_U_ADD_IMM_FMT_3(uint8_t, 9)
+DEF_SAT_U_ADD_IMM_FMT_4(uint8_t, 9)
+
+/* { dg-final { scan-rtl-dump-times ".SAT_ADD " 4 "expand" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-2.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-2.c
new file mode 100644
index 00000000000..d5de0960a96
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-2.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
+
+#include "sat_arith.h"
+
+DEF_SAT_U_ADD_IMM_FMT_3(uint16_t, 91)
+DEF_SAT_U_ADD_IMM_FMT_4(uint16_t, 91)
+
+/* { dg-final { scan-rtl-dump-times ".SAT_ADD " 4 "expand" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-3.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-3.c
new file mode 100644
index 00000000000..a3391ca460a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-3.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
+
+#include "sat_arith.h"
+
+DEF_SAT_U_ADD_IMM_FMT_3(uint32_t, 191u)
+DEF_SAT_U_ADD_IMM_FMT_4(uint32_t, 191u)
+
+/* { dg-final { scan-rtl-dump-times ".SAT_ADD " 4 "expand" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-4.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-4.c
new file mode 100644
index 00000000000..bc12e0e346f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-4.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
+
+#include "sat_arith.h"
+
+DEF_SAT_U_ADD_IMM_FMT_3(uint64_t, 191)
+DEF_SAT_U_ADD_IMM_FMT_4(uint64_t, 191)
+
+/* { dg-final { scan-rtl-dump-not ".SAT_ADD " "expand" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-5.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-5.c
new file mode 100644
index 00000000000..9b92d9624b5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-5.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
+
+#include "sat_arith.h"
+
+DEF_SAT_U_ADD_IMM_FMT_3(uint64_t, 191ull)
+DEF_SAT_U_ADD_IMM_FMT_4(uint64_t, 191ull)
+
+/* { dg-final { scan-rtl-dump-times ".SAT_ADD " 4 "expand" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-6.c b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-6.c
new file mode 100644
index 00000000000..a64f417bda3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/sat_u_add_imm_type_check-6.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64d -O3 -fdump-rtl-expand-details" } */
+
+#include "sat_arith.h"
+
+DEF_SAT_U_ADD_IMM_FMT_3(uint32_t, 191ull)
+DEF_SAT_U_ADD_IMM_FMT_4(uint32_t, 191ull)
+
+/* { dg-final { scan-rtl-dump-not ".SAT_ADD " "expand" } } */