On Wed, Jul 17, 2024 at 1:03 PM Tamar Christina <tamar.christ...@arm.com> wrote:
>
> > -----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.
So type_has_mode_precision_p only works with scalar integral types
(maybe scalar real types too) since it uses TYPE_PRECISION directly
and not element_precision (the precision field is overloaded for
vectors for the number of elements and TYPE_PRECISION on a vector type
will cause an ICE since r14-2150-gfe48f2651334bc).
So I suspect you need to check !VECTOR_TYPE_P (type) before calling
type_has_mode_precision_p .
Thanks,
Andrew Pinski
>
> 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
> > >>
