| Message ID | 1408327259-23386-1-git-send-email-patrick@parcs.ath.cx |
|---|---|
| State | New |
| Headers | show |
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 >
On Mon, Aug 18, 2014 at 6:48 AM, Richard Biener <richard.guenther@gmail.com> wrote: > 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. Unfortunately I lied when I optimistically said that regression testing passed. This patch makes causes a single new regression in gcc.target/i386/pr14289-1.c. The file looks like: /* { dg-options "-O0" } */ register int n[2] asm ("ebx"); void foo (void) { int i = 0; a[i] = 0; /* { dg-error "address of" } */ } Whereas previously the C FE would have reported an error for trying to set the TREE_ADDRESSABLE bit on a register variable, now an ICE is triggered during RTL expansion for trying to index into a register array with a non-constant index. So I am thinking of explicitly checking for this scenario and emitting an error. Do you think such a check should be done in the frontends or during RTL expansion? If done during RTL expansion, the optimizers could have a chance of turning the non-constant index into a constant one like in this example.
On Mon, Aug 18, 2014 at 2:31 PM, Patrick Palka <patrick@parcs.ath.cx> wrote: > On Mon, Aug 18, 2014 at 6:48 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> 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. Unfortunately I lied when I optimistically said that > regression testing passed. This patch makes causes a single new > regression in gcc.target/i386/pr14289-1.c. The file looks like: > > /* { dg-options "-O0" } */ > > register int n[2] asm ("ebx"); > > void > foo (void) > { > int i = 0; > a[i] = 0; /* { dg-error "address of" } */ > } > > Whereas previously the C FE would have reported an error for trying to > set the TREE_ADDRESSABLE bit on a register variable, now an ICE is > triggered during RTL expansion for trying to index into a register > array with a non-constant index. So I am thinking of explicitly > checking for this scenario and emitting an error. Do you think such a > check should be done in the frontends or during RTL expansion? If > done during RTL expansion, the optimizers could have a chance of > turning the non-constant index into a constant one like in this > example. I think emitting the error in the above case is to avoid ICEs for strange cases. We _should_ be able to expand a[i] = 0 even with variable index - if only by spilling 'ebx' to memory, changing it as an array and then loading it back to 'ebx'. Thus, fixing the ICE would be more welcome here. Other than that, yes, erroring in the frontend for non-constant indexed register variables is ok with me as well. The above also errors for -O1, right? Richard.
On Mon, Aug 18, 2014 at 8:50 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Mon, Aug 18, 2014 at 2:31 PM, Patrick Palka <patrick@parcs.ath.cx> wrote: >> On Mon, Aug 18, 2014 at 6:48 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> 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. Unfortunately I lied when I optimistically said that >> regression testing passed. This patch makes causes a single new >> regression in gcc.target/i386/pr14289-1.c. The file looks like: >> >> /* { dg-options "-O0" } */ >> >> register int n[2] asm ("ebx"); >> >> void >> foo (void) >> { >> int i = 0; >> a[i] = 0; /* { dg-error "address of" } */ >> } >> >> Whereas previously the C FE would have reported an error for trying to >> set the TREE_ADDRESSABLE bit on a register variable, now an ICE is >> triggered during RTL expansion for trying to index into a register >> array with a non-constant index. So I am thinking of explicitly >> checking for this scenario and emitting an error. Do you think such a >> check should be done in the frontends or during RTL expansion? If >> done during RTL expansion, the optimizers could have a chance of >> turning the non-constant index into a constant one like in this >> example. > > I think emitting the error in the above case is to avoid ICEs for > strange cases. We _should_ be able to expand > a[i] = 0 even with variable index - if only by spilling 'ebx' to memory, > changing it as an array and then loading it back to 'ebx'. Good point. > > Thus, fixing the ICE would be more welcome here. Other than that, > yes, erroring in the frontend for non-constant indexed register > variables is ok with me as well. The above also errors for -O1, right? With the patch, the above doesn't error for -O1 because a[i] is trivially optimized to a[0] -- a constant index -- before RTL expansion. I'll take a stab at fixing the ICE, then.
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" } } */
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? 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