On Wed, Jul 17, 2024 at 4:13 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> 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?
I was thinking it should be in
direct_optab_supported_p/convert_optab_supported_p/direct_internal_fn_optab
too when I did my initial analysis of the bug report. I tried to see
if there was another use of direct_optab_supported_p where this could
happen but I didn't find one.
Thanks,
Andrew
>
> 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
> >
