Re: [PATCH 1/2] gcc: Remove unneeded global flag.

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Jeff Law <law@redhat.com> [2016-06-21 20:55:15 -0600]:

> On 06/10/2016 10:56 AM, Andrew Burgess wrote:
> > The global flag `user_defined_section_attribute' is set while parsing C
> > code when the section attribute is encountered.  The flag is set when
> > anything has the section attribute applied to it, functions or data.
> > 
> > The only place this global was used was within the gate function for
> > partitioning blocks (pass_partition_blocks::gate), however, the
> > partitioning is done during compilation, while the flag is set earlier,
> > during the parsing.  The flag is then cleared again during the final
> > compilation pass.
> > 
> > The result is that if any function or data has a section attribute then
> > the flag will be set to true during the file parse pass.  The first
> > compiled function will then skip the partition-blocks pass, and the flag
> > will be set back to false during the final-pass on the first function.
> > After then, the flag is never set to true again.
> > 
> > The guarding of the partition-blocks pass does not appear to be
> > necessary, given that applying a section attribute correctly
> > overrides the hot/cold section partitioning (this is taken care if in
> > varasm.c).
> > 
> > gcc/ChangeLog:
> > 
> > 	* gcc/bb-reorder.c: Remove 'toplev.h' include.
> > 	(pass_partition_blocks::gate): No longer check
> > 	user_defined_section_attribute.
> > 	* gcc/c-family/c-common.c (handle_section_attribute): No longer
> > 	set user_defined_section_attribute.
> > 	* gcc/final.c (rest_of_handle_final): Likewise.
> > 	* gcc/toplev.c: Remove definition of user_defined_section_attribute.
> > 	* gcc/toplev.h: Remove declaration of
> > 	user_defined_section_attribute.
> user_defined_section_attribute was introduced as part of the hot/cold
> partitioning changes.
> 
> https://gcc.gnu.org/ml/gcc-patches/2004-07/msg01545.html
> 
> 
> What's supposed to happen is hot/cold partitioning is supposed to be turned
> off for the function which has the a user defined section attribute.
> 
> So proper behaviour is to set the flag to true when the attribute is parsed
> and turn it off when we're finished with the current function. The gate for
> hot/cold partitioning should check the value of the flag and avoid hot/cold
> partitioning when the flag is true.
> 
> So AFAICT everything is working as it should.  Keep in mind that multiple
> functions might have user defined section attributes.

Jeff & Jakub,

Thanks for taking the time to review and provide feedback.  Let me
explain how I _think_ it's working at the moment, then you can point
out where I might be going wrong.

My understanding is that currently all functions are parsed, and then
all functions are optimised / compiled.

The user_defined_section_attribute variable is initialised to false,
and set true when the section attribute is parsed.  However, as
pass_partition_blocks::gate is called as part of the optimise /
compile process we only read this variable once all of the functions
have been parsed.

The user_defined_section_attribute variable is reset to false in
rest_of_handle_final, which is the final compile / optimise pass.  So
I think how it (doesn't) currently work is that
user_defined_section_attribute starts as false, then if _any_ function
has a user defined section attribute the variable is set true.  Then,
in the compiler / optimise phase the _first_ function will not perform
the partition-blocks pass, after which user_defined_section_attribute
will be reset to false, and all future blocks will go through the
partition-blocks pass.

In the original patch I simply removed user_defined_section_attribute
figuring that given we've been happily running the partition-blocks
pass then this can't be too harmful, however, as Jakub said perhaps I
should have implemented a correct check.

I've attached an updated patch to remove
user_defined_section_attribute and replace the check in
pass_partition_blocks::gate with one the looks for the section
attribute on the function decl.

Feedback welcome,

Thanks,
Andrew

---

gcc: Remove unneeded global flag

The global flag `user_defined_section_attribute' is set while parsing C
code when the section attribute is encountered.  The flag is set when
anything has the section attribute applied to it, functions or data.

The only place this global was used was within the gate function for
partitioning blocks (pass_partition_blocks::gate), however, the
partitioning is done during compilation, while the flag is set earlier,
during the parsing.  The flag is then cleared again during the final
compilation pass.

The result is that if any function or data has a section attribute then
the flag will be set to true during the file parse pass.  The first
compiled function will then skip the partition-blocks pass, and the flag
will be set back to false during the final-pass on the first function.
After then, the flag is never set to true again.

The guarding of the partition-blocks pass does not appear to be
necessary, given that applying a section attribute correctly
overrides the hot/cold section partitioning (this is taken care if in
varasm.c).

gcc/ChangeLog:

	* gcc/bb-reorder.c: Remove 'toplev.h' include.
	(pass_partition_blocks::gate): No longer check
	user_defined_section_attribute, instead check the function decl
	for a section attribute.
	* gcc/c-family/c-common.c (handle_section_attribute): No longer
	set user_defined_section_attribute.
	* gcc/final.c (rest_of_handle_final): Likewise.
	* gcc/toplev.c: Remove definition of user_defined_section_attribute.
	* gcc/toplev.h: Remove declaration of
	user_defined_section_attribute.
---
 gcc/bb-reorder.c        | 3 +--
 gcc/c-family/c-common.c | 2 --
 gcc/final.c             | 2 --
 gcc/toplev.c            | 5 -----
 gcc/toplev.h            | 5 -----
 5 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c
index bb8435f..0a482e6 100644
--- a/gcc/bb-reorder.c
+++ b/gcc/bb-reorder.c
@@ -106,7 +106,6 @@
 #include "output.h"
 #include "expr.h"
 #include "params.h"
-#include "toplev.h" /* user_defined_section_attribute */
 #include "tree-pass.h"
 #include "cfgrtl.h"
 #include "cfganal.h"
@@ -2890,7 +2889,7 @@ pass_partition_blocks::gate (function *fun)
 	     we are going to omit the reordering.  */
 	  && optimize_function_for_speed_p (fun)
 	  && !DECL_COMDAT_GROUP (current_function_decl)
-	  && !user_defined_section_attribute);
+	  && !lookup_attribute ("section", DECL_ATTRIBUTES (fun->decl)));
 }
 
 unsigned
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3301c31..1f403d5 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -7619,8 +7619,6 @@ handle_section_attribute (tree *node, tree ARG_UNUSED (name), tree args,
       goto fail;
     }
 
-  user_defined_section_attribute = true;
-
   if (!VAR_OR_FUNCTION_DECL_P (decl))
     {
       error ("section attribute not allowed for %q+D", *node);
diff --git a/gcc/final.c b/gcc/final.c
index 5b04311..256ce34 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -4454,8 +4454,6 @@ rest_of_handle_final (void)
 
   assemble_end_function (current_function_decl, fnname);
 
-  user_defined_section_attribute = false;
-
   /* Free up reg info memory.  */
   free_reg_info ();
 
diff --git a/gcc/toplev.c b/gcc/toplev.c
index da80097..648403e 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -148,11 +148,6 @@ HOST_WIDE_INT random_seed;
    the support provided depends on the backend.  */
 rtx stack_limit_rtx;
 
-/* True if the user has tagged the function with the 'section'
-   attribute.  */
-
-bool user_defined_section_attribute = false;
-
 struct target_flag_state default_target_flag_state;
 #if SWITCHABLE_TARGET
 struct target_flag_state *this_target_flag_state = &default_target_flag_state;
diff --git a/gcc/toplev.h b/gcc/toplev.h
index 06923cf..f62a172 100644
--- a/gcc/toplev.h
+++ b/gcc/toplev.h
@@ -74,11 +74,6 @@ extern void target_reinit (void);
 /* A unique local time stamp, might be zero if none is available.  */
 extern unsigned local_tick;
 
-/* True if the user has tagged the function with the 'section'
-   attribute.  */
-
-extern bool user_defined_section_attribute;
-
 /* See toplev.c.  */
 extern int flag_rerun_cse_after_global_opts;
 
-- 
2.5.1