> -----Original Message-----
> From: Richard Sandiford <richard.sandif...@arm.com>
> Sent: Wednesday, July 17, 2024 8:55 PM
> To: Richard Biener <richard.guent...@gmail.com>
> Cc: pan2...@intel.com; gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai;
> kito.ch...@gmail.com; Tamar Christina <tamar.christ...@arm.com>;
> jeffreya...@gmail.com; rdapp....@gmail.com; hongtao....@intel.com
> Subject: Re: [PATCH v1] Match: Bugfix .SAT_TRUNC honor types has no mode
> precision [PR115961]
>
> Richard Biener <richard.guent...@gmail.com> writes:
> > On Wed, Jul 17, 2024 at 11:48 AM <pan2...@intel.com> wrote:
> >>
> >> From: Pan Li <pan2...@intel.com>
> >>
> >> The .SAT_TRUNC matching doesn't check the type has mode precision. Thus
> >> when bitfield like below will be recog as .SAT_TRUNC.
> >>
> >> struct e
> >> {
> >> unsigned pre : 12;
> >> unsigned a : 4;
> >> };
> >>
> >> __attribute__((noipa))
> >> void bug (e * v, unsigned def, unsigned use) {
> >> e & defE = *v;
> >> defE.a = min_u (use + 1, 0xf);
> >> }
> >>
> >> This patch would like to add type_has_mode_precision_p for the
> >> .SAT_TRUNC matching to get rid of this.
> >>
> >> The below test suites are passed for this patch:
> >> 1. The rv64gcv fully regression tests.
> >> 2. The x86 bootstrap tests.
> >> 3. The x86 fully regression tests.
> >
> > Hmm, rather than restricting the matching the issue is the optab query or
> > in this case how *_optab_supported_p blindly uses TYPE_MODE without
> > either asserting the type has mode precision or failing the query in this
> > case.
> >
> > I think it would be simplest to adjust direct_optab_supported_p
> > (and convert_optab_supported_p) to reject such operations? Richard, do
> > you agree or should callers check this instead?
>
> Sounds good to me, although I suppose it should go:
>
> bool
> direct_internal_fn_supported_p (internal_fn fn, tree_pair types,
> optimization_type opt_type)
> {
> // <--- Here
> switch (fn)
> {
>
> }
> }
>
> until we know of a specific case where that's wrong.
>
> Is type_has_mode_precision_p meaningful for all types?
>
I was wondering about that, wouldn't VECTOR_BOOLEAN_TYPE_P types fail?
e.g. on AVX where the type precision is 1 but the mode precision QImode?
Unless I misunderstood the predicate.
Regards,
Tamar
> Richard
>
>
> >
> > So, instead of match.pd the check would need to be in vector pattern
> > matching
> > and SSA math opts. Or alternatively in internal-fn.cc as laid out above.
> >
> > Richard.
> >
> >> PR target/115961
> >>
> >> gcc/ChangeLog:
> >>
> >> * match.pd: Add type_has_mode_precision_p check for .SAT_TRUNC.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> * g++.target/i386/pr115961-run-1.C: New test.
> >> * g++.target/riscv/rvv/base/pr115961-run-1.C: New test.
> >>
> >> Signed-off-by: Pan Li <pan2...@intel.com>
> >> ---
> >> gcc/match.pd | 4 +--
> >> .../g++.target/i386/pr115961-run-1.C | 34 +++++++++++++++++++
> >> .../riscv/rvv/base/pr115961-run-1.C | 34 +++++++++++++++++++
> >> 3 files changed, 70 insertions(+), 2 deletions(-)
> >> create mode 100644 gcc/testsuite/g++.target/i386/pr115961-run-1.C
> >> create mode 100644 gcc/testsuite/g++.target/riscv/rvv/base/pr115961-run-
> 1.C
> >>
> >> diff --git a/gcc/match.pd b/gcc/match.pd
> >> index 24a0bbead3e..8121ec09f53 100644
> >> --- a/gcc/match.pd
> >> +++ b/gcc/match.pd
> >> @@ -3240,7 +3240,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >> (bit_ior:c (negate (convert (gt @0 INTEGER_CST@1)))
> >> (convert @0))
> >> (if (INTEGRAL_TYPE_P (type) && TYPE_UNSIGNED (type)
> >> - && TYPE_UNSIGNED (TREE_TYPE (@0)))
> >> + && TYPE_UNSIGNED (TREE_TYPE (@0)) && type_has_mode_precision_p
> (type))
> >> (with
> >> {
> >> unsigned itype_precision = TYPE_PRECISION (TREE_TYPE (@0));
> >> @@ -3255,7 +3255,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >> (match (unsigned_integer_sat_trunc @0)
> >> (convert (min @0 INTEGER_CST@1))
> >> (if (INTEGRAL_TYPE_P (type) && TYPE_UNSIGNED (type)
> >> - && TYPE_UNSIGNED (TREE_TYPE (@0)))
> >> + && TYPE_UNSIGNED (TREE_TYPE (@0)) && type_has_mode_precision_p
> (type))
> >> (with
> >> {
> >> unsigned itype_precision = TYPE_PRECISION (TREE_TYPE (@0));
> >> diff --git a/gcc/testsuite/g++.target/i386/pr115961-run-1.C
> b/gcc/testsuite/g++.target/i386/pr115961-run-1.C
> >> new file mode 100644
> >> index 00000000000..b8c8aef3b17
> >> --- /dev/null
> >> +++ b/gcc/testsuite/g++.target/i386/pr115961-run-1.C
> >> @@ -0,0 +1,34 @@
> >> +/* PR target/115961 */
> >> +/* { dg-do run } */
> >> +/* { dg-options "-O3 -fdump-rtl-expand-details" } */
> >> +
> >> +struct e
> >> +{
> >> + unsigned pre : 12;
> >> + unsigned a : 4;
> >> +};
> >> +
> >> +static unsigned min_u (unsigned a, unsigned b)
> >> +{
> >> + return (b < a) ? b : a;
> >> +}
> >> +
> >> +__attribute__((noipa))
> >> +void bug (e * v, unsigned def, unsigned use) {
> >> + e & defE = *v;
> >> + defE.a = min_u (use + 1, 0xf);
> >> +}
> >> +
> >> +__attribute__((noipa, optimize(0)))
> >> +int main(void)
> >> +{
> >> + e v = { 0xded, 3 };
> >> +
> >> + bug(&v, 32, 33);
> >> +
> >> + if (v.a != 0xf)
> >> + __builtin_abort ();
> >> +
> >> + return 0;
> >> +}
> >> +/* { dg-final { scan-rtl-dump-not ".SAT_TRUNC " "expand" } } */
> >> diff --git a/gcc/testsuite/g++.target/riscv/rvv/base/pr115961-run-1.C
> b/gcc/testsuite/g++.target/riscv/rvv/base/pr115961-run-1.C
> >> new file mode 100644
> >> index 00000000000..b8c8aef3b17
> >> --- /dev/null
> >> +++ b/gcc/testsuite/g++.target/riscv/rvv/base/pr115961-run-1.C
> >> @@ -0,0 +1,34 @@
> >> +/* PR target/115961 */
> >> +/* { dg-do run } */
> >> +/* { dg-options "-O3 -fdump-rtl-expand-details" } */
> >> +
> >> +struct e
> >> +{
> >> + unsigned pre : 12;
> >> + unsigned a : 4;
> >> +};
> >> +
> >> +static unsigned min_u (unsigned a, unsigned b)
> >> +{
> >> + return (b < a) ? b : a;
> >> +}
> >> +
> >> +__attribute__((noipa))
> >> +void bug (e * v, unsigned def, unsigned use) {
> >> + e & defE = *v;
> >> + defE.a = min_u (use + 1, 0xf);
> >> +}
> >> +
> >> +__attribute__((noipa, optimize(0)))
> >> +int main(void)
> >> +{
> >> + e v = { 0xded, 3 };
> >> +
> >> + bug(&v, 32, 33);
> >> +
> >> + if (v.a != 0xf)
> >> + __builtin_abort ();
> >> +
> >> + return 0;
> >> +}
> >> +/* { dg-final { scan-rtl-dump-not ".SAT_TRUNC " "expand" } } */
> >> --
> >> 2.34.1
> >>
