Re: [PATCH] tree-optimization/111779 - Handle some BIT_FIELD_REFs in SRA

[Andre Vehreschild <vehre@gmx.de>]

Hi Richard,

being the one who wrote the surrounding code:
The fortran part looks good to me.

Ok for merge from the fortran side.

- Andre

On Thu, 12 Oct 2023 11:44:01 +0000 (UTC)
Richard Biener <rguenther@suse.de> wrote:

> On Thu, 12 Oct 2023, Richard Biener wrote:
>
> > The following handles byte-aligned, power-of-two and byte-multiple
> > sized BIT_FIELD_REF reads in SRA.  In particular this should cover
> > BIT_FIELD_REFs created by optimize_bit_field_compare.
> >
> > For gcc.dg/tree-ssa/ssa-dse-26.c we now SRA the BIT_FIELD_REF
> > appearing there leading to more DSE, fully eliding the aggregates.
> >
> > This results in the same false positive -Wuninitialized as the
> > older attempt to remove the folding from optimize_bit_field_compare,
> > fixed by initializing part of the aggregate unconditionally.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu for all languages.
> >
> > Martin is on leave so I'll push this tomorrow unless the Fortran
> > folks have objections.
>
> Err, and I forgot that hunk.  It's
>
> diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
> index 7beefa2e69c..1b8be081a17 100644
> --- a/gcc/fortran/trans-expr.cc
> +++ b/gcc/fortran/trans-expr.cc
> @@ -12015,7 +12015,10 @@ gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr *
> expr2, bool init_flag, && !is_runtime_conformable (expr1, expr2);
>
>    /* Only analyze the expressions for coarray properties, when in coarray-lib
> -     mode.  */
> +     mode.  Avoid false-positive uninitialized diagnostics with initializing
> +     the codimension flag unconditionally.  */
> +  lhs_caf_attr.codimension = false;
> +  rhs_caf_attr.codimension = false;
>    if (flag_coarray == GFC_FCOARRAY_LIB)
>      {
>        lhs_caf_attr = gfc_caf_attr (expr1, false, &lhs_refs_comp);
>
>
> > Thanks,
> > Richard.
> >
> > 	PR tree-optimization/111779
> > gcc/
> > 	* tree-sra.cc (sra_handled_bf_read_p): New function.
> > 	(build_access_from_expr_1): Handle some BIT_FIELD_REFs.
> > 	(sra_modify_expr): Likewise.
> > 	(make_fancy_name_1): Skip over BIT_FIELD_REF.
> >
> > gcc/fortran/
> > 	* trans-expr.cc (gfc_trans_assignment_1): Initialize
> > 	lhs_caf_attr and rhs_caf_attr codimension flag to avoid
> > 	false positive -Wuninitialized.
> >
> > gcc/testsuite/
> > 	* gcc.dg/tree-ssa/ssa-dse-26.c: Adjust for more DSE.
> > 	* gcc.dg/vect/vect-pr111779.c: New testcase.
> > ---
> >  gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-26.c |  4 +-
> >  gcc/testsuite/gcc.dg/vect/vect-pr111779.c  | 56 ++++++++++++++++++++++
> >  gcc/tree-sra.cc                            | 24 ++++++++--
> >  3 files changed, 79 insertions(+), 5 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.dg/vect/vect-pr111779.c
> >
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-26.c
> > b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-26.c index e3c33f49ef6..43152de5616
> > 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-26.c
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-26.c
> > @@ -31,5 +31,5 @@ constraint_equal (struct constraint a, struct constraint
> > b) && constraint_expr_equal (a.rhs, b.rhs);
> >  }
> >
> > -/* { dg-final { scan-tree-dump-times "Deleted dead store: x = " 1 "dse1" }
> > } */ -/* { dg-final { scan-tree-dump-times "Deleted dead store: y = " 1
> > "dse1" } } */ +/* { dg-final { scan-tree-dump-times "Deleted dead store: x
> > = " 2 "dse1" } } */ +/* { dg-final { scan-tree-dump-times "Deleted dead
> > store: y = " 2 "dse1" } } */ diff --git
> > a/gcc/testsuite/gcc.dg/vect/vect-pr111779.c
> > b/gcc/testsuite/gcc.dg/vect/vect-pr111779.c new file mode 100644 index
> > 00000000000..79b72aebc78 --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/vect/vect-pr111779.c
> > @@ -0,0 +1,56 @@
> > +#include <stdbool.h>
> > +#include "tree-vect.h"
> > +
> > +struct C
> > +{
> > +    int c;
> > +    int d;
> > +    bool f :1;
> > +    float e;
> > +};
> > +
> > +struct A
> > +{
> > +  unsigned int a;
> > +  unsigned char c1, c2;
> > +  bool b1 : 1;
> > +  bool b2 : 1;
> > +  bool b3 : 1;
> > +  struct C b4;
> > +};
> > +
> > +void __attribute__((noipa))
> > +foo (const struct A * __restrict x, int y)
> > +{
> > +  int s = 0, i = 0;
> > +  for (i = 0; i < y; ++i)
> > +    {
> > +      const struct A a = x[i];
> > +      s += a.b4.f ? 1 : 0;
> > +    }
> > +  if (s != 0)
> > +    __builtin_abort ();
> > +}
> > +
> > +int
> > +main ()
> > +{
> > +  struct A x[100];
> > +  int i;
> > +
> > +  check_vect ();
> > +
> > +  __builtin_memset (x, -1, sizeof (x));
> > +#pragma GCC novect
> > +  for (i = 0; i < 100; i++)
> > +    {
> > +      x[i].b1 = false;
> > +      x[i].b2 = false;
> > +      x[i].b3 = false;
> > +      x[i].b4.f = false;
> > +    }
> > +  foo (x, 100);
> > +  return 0;
> > +}
> > +
> > +/* { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" { target vect_int
> > } } } */ diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
> > index 56a8ba26135..24d0c20da6a 100644
> > --- a/gcc/tree-sra.cc
> > +++ b/gcc/tree-sra.cc
> > @@ -1113,6 +1113,21 @@ disqualify_base_of_expr (tree t, const char *reason)
> >      disqualify_candidate (t, reason);
> >  }
> >
> > +/* Return true if the BIT_FIELD_REF read EXPR is handled by SRA.  */
> > +
> > +static bool
> > +sra_handled_bf_read_p (tree expr)
> > +{
> > +  uint64_t size, offset;
> > +  if (bit_field_size (expr).is_constant (&size)
> > +      && bit_field_offset (expr).is_constant (&offset)
> > +      && size % BITS_PER_UNIT == 0
> > +      && offset % BITS_PER_UNIT == 0
> > +      && pow2p_hwi (size))
> > +    return true;
> > +  return false;
> > +}
> > +
> >  /* Scan expression EXPR and create access structures for all accesses to
> >     candidates for scalarization.  Return the created access or NULL if
> > none is created.  */
> > @@ -1123,7 +1138,8 @@ build_access_from_expr_1 (tree expr, gimple *stmt,
> > bool write) struct access *ret = NULL;
> >    bool partial_ref;
> >
> > -  if (TREE_CODE (expr) == BIT_FIELD_REF
> > +  if ((TREE_CODE (expr) == BIT_FIELD_REF
> > +       && (write || !sra_handled_bf_read_p (expr)))
> >        || TREE_CODE (expr) == IMAGPART_EXPR
> >        || TREE_CODE (expr) == REALPART_EXPR)
> >      {
> > @@ -1170,6 +1186,7 @@ build_access_from_expr_1 (tree expr, gimple *stmt,
> > bool write) case COMPONENT_REF:
> >      case ARRAY_REF:
> >      case ARRAY_RANGE_REF:
> > +    case BIT_FIELD_REF:
> >        ret = create_access (expr, stmt, write);
> >        break;
> >
> > @@ -1549,6 +1566,7 @@ make_fancy_name_1 (tree expr)
> >        obstack_grow (&name_obstack, buffer, strlen (buffer));
> >        break;
> >
> > +    case BIT_FIELD_REF:
> >      case ADDR_EXPR:
> >        make_fancy_name_1 (TREE_OPERAND (expr, 0));
> >        break;
> > @@ -1564,7 +1582,6 @@ make_fancy_name_1 (tree expr)
> >  	}
> >        break;
> >
> > -    case BIT_FIELD_REF:
> >      case REALPART_EXPR:
> >      case IMAGPART_EXPR:
> >        gcc_unreachable (); 	/* we treat these as scalars.  */
> > @@ -3769,7 +3786,8 @@ sra_modify_expr (tree *expr, gimple_stmt_iterator
> > *gsi, bool write) tree type, bfr, orig_expr;
> >    bool partial_cplx_access = false;
> >
> > -  if (TREE_CODE (*expr) == BIT_FIELD_REF)
> > +  if (TREE_CODE (*expr) == BIT_FIELD_REF
> > +      && (write || !sra_handled_bf_read_p (*expr)))
> >      {
> >        bfr = *expr;
> >        expr = &TREE_OPERAND (*expr, 0);
> >
>


--
Andre Vehreschild * Email: vehre ad gmx dot de