Re: [PATCH] Fix promotion of const local arrays to static storage

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Aug 18, 2014 at 4:00 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> Hi,
>
> The fix for PR38615 indirectly broke the promotion of const local arrays
> to static storage in many cases.  The commit in question, r143570, made
> it so that only arrays that don't potentially escape from the scope in
> which they're defined (i.e. arrays for which TREE_ADDRESSABLE is 0) are
> candidates for the promotion to static storage.
>
> The problem is that both the C and C++ frontends contain ancient code
> (dating back to 1994) that sets the TREE_ADDRESSABLE bit on arrays
> indexed by non-constant or out-of-range indices.  As a result, const
> arrays that are indexed by non-constant or out-of-range values are no
> longer candidates for promotion to static storage following the fix to
> PR38615, because their TREE_ADDRESSABLE bit will always be set.
> Consequently, array promotion is essentially broken for a large class of
> C and C++ code.  E.g. this simple example is no longer a candidate for
> promotion:
>
>     int
>     foo (int i)
>     {
>       const int x[] = { 1, 2, 3 };
>       return x[i]; /* non-constant index */
>     }
>
> This patch removes the ancient code that is responsible for dubiously
> setting the TREE_ADDRESSABLE bit on arrays indexed by non-constant or
> out-of-range values.  I don't think that this code is necessary or
> useful anymore.  Bootstrapped and regtested on x86_64-unknown-linux-gnu,
> OK for trunk?

This looks good to me - indeed TREE_ADDRESSABLE on variable-indexed
things isn't necessary (and before that it was made sure to re-set it
before RTL expansion which required it, as update-address-taken would
have happily removed TREE_ADDRESSABLE anyway).

Thanks,
Richard.

> 2014-08-18  Patrick Palka  <ppalka@gcc.gnu.org>
>
>         * c-typeck.c (build_array_ref): Don't mark arrays indexed by
>         non-constant or out-of-range values as addressable.
>
> 2014-08-18  Patrick Palka  <ppalka@gcc.gnu.org>
>
>         * typeck.c (build_array_ref): Don't mark arrays indexed by
>         non-constant or out-of-range values as addressable.
>
> 2014-08-18  Patrick Palka  <ppalka@gcc.gnu.org>
>
>         * g++.dg/opt/static4.C: Check for static promotion.
>         * g++.dg/opt/static7.C: New test.
>         * gcc.dg/static1.c: New test.
> ---
>  gcc/c/c-typeck.c                   | 23 -----------------------
>  gcc/cp/typeck.c                    | 25 -------------------------
>  gcc/testsuite/g++.dg/opt/static4.C |  4 ++++
>  gcc/testsuite/g++.dg/opt/static7.C | 12 ++++++++++++
>  gcc/testsuite/gcc.dg/static1.c     | 12 ++++++++++++
>  5 files changed, 28 insertions(+), 48 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/opt/static7.C
>  create mode 100644 gcc/testsuite/gcc.dg/static1.c
>
> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index e6745be..022ff8d 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -2487,29 +2487,6 @@ build_array_ref (location_t loc, tree array, tree index)
>      {
>        tree rval, type;
>
> -      /* An array that is indexed by a non-constant
> -        cannot be stored in a register; we must be able to do
> -        address arithmetic on its address.
> -        Likewise an array of elements of variable size.  */
> -      if (TREE_CODE (index) != INTEGER_CST
> -         || (COMPLETE_TYPE_P (TREE_TYPE (TREE_TYPE (array)))
> -             && TREE_CODE (TYPE_SIZE (TREE_TYPE (TREE_TYPE (array)))) != INTEGER_CST))
> -       {
> -         if (!c_mark_addressable (array))
> -           return error_mark_node;
> -       }
> -      /* An array that is indexed by a constant value which is not within
> -        the array bounds cannot be stored in a register either; because we
> -        would get a crash in store_bit_field/extract_bit_field when trying
> -        to access a non-existent part of the register.  */
> -      if (TREE_CODE (index) == INTEGER_CST
> -         && TYPE_DOMAIN (TREE_TYPE (array))
> -         && !int_fits_type_p (index, TYPE_DOMAIN (TREE_TYPE (array))))
> -       {
> -         if (!c_mark_addressable (array))
> -           return error_mark_node;
> -       }
> -
>        if (pedantic || warn_c90_c99_compat)
>         {
>           tree foo = array;
> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> index fa3283d..6dd056d 100644
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -3112,31 +3112,6 @@ cp_build_array_ref (location_t loc, tree array, tree idx,
>          pointer arithmetic.)  */
>        idx = cp_perform_integral_promotions (idx, complain);
>
> -      /* An array that is indexed by a non-constant
> -        cannot be stored in a register; we must be able to do
> -        address arithmetic on its address.
> -        Likewise an array of elements of variable size.  */
> -      if (TREE_CODE (idx) != INTEGER_CST
> -         || (COMPLETE_TYPE_P (TREE_TYPE (TREE_TYPE (array)))
> -             && (TREE_CODE (TYPE_SIZE (TREE_TYPE (TREE_TYPE (array))))
> -                 != INTEGER_CST)))
> -       {
> -         if (!cxx_mark_addressable (array))
> -           return error_mark_node;
> -       }
> -
> -      /* An array that is indexed by a constant value which is not within
> -        the array bounds cannot be stored in a register either; because we
> -        would get a crash in store_bit_field/extract_bit_field when trying
> -        to access a non-existent part of the register.  */
> -      if (TREE_CODE (idx) == INTEGER_CST
> -         && TYPE_DOMAIN (TREE_TYPE (array))
> -         && ! int_fits_type_p (idx, TYPE_DOMAIN (TREE_TYPE (array))))
> -       {
> -         if (!cxx_mark_addressable (array))
> -           return error_mark_node;
> -       }
> -
>        if (!lvalue_p (array))
>         {
>           if (complain & tf_error)
> diff --git a/gcc/testsuite/g++.dg/opt/static4.C b/gcc/testsuite/g++.dg/opt/static4.C
> index 87e11b0..bae22c9 100644
> --- a/gcc/testsuite/g++.dg/opt/static4.C
> +++ b/gcc/testsuite/g++.dg/opt/static4.C
> @@ -3,6 +3,7 @@
>  // if they are promoted to static storage.
>
>  // { dg-do compile }
> +// { dg-options "-fdump-tree-gimple" }
>
>  int g(int i) {
>    if (i<1) {
> @@ -13,3 +14,6 @@ int g(int i) {
>      return x[i];
>    }
>  }
> +
> +// { dg-final { scan-tree-dump-times "static const int" 2 "gimple" } }
> +// { dg-final { cleanup-tree-dump "gimple" } }
> diff --git a/gcc/testsuite/g++.dg/opt/static7.C b/gcc/testsuite/g++.dg/opt/static7.C
> new file mode 100644
> index 0000000..2a9f678
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/opt/static7.C
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fdump-tree-gimple" } */
> +
> +int
> +foo (int x)
> +{
> +  const int y[] = { 1, 2, 3 };
> +  return y[x];
> +}
> +
> +/* { dg-final { scan-tree-dump "static const int" "gimple" } } */
> +/* { dg-final { cleanup-tree-dump "gimple" } } */
> diff --git a/gcc/testsuite/gcc.dg/static1.c b/gcc/testsuite/gcc.dg/static1.c
> new file mode 100644
> index 0000000..2a9f678
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/static1.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fdump-tree-gimple" } */
> +
> +int
> +foo (int x)
> +{
> +  const int y[] = { 1, 2, 3 };
> +  return y[x];
> +}
> +
> +/* { dg-final { scan-tree-dump "static const int" "gimple" } } */
> +/* { dg-final { cleanup-tree-dump "gimple" } } */
> --
> 2.1.0
>