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

In an attempt to get this patch merged (as I still think that its
correct) I've investigated, and documented a little more about how I
think things currently work.  I'm sure most people reading this will
already know this, but hopefully, if my understanding is wrong someone
can point it out.

I've updated the patch to include a few tests, however, I have a
little concern about the new test as they use '.text' and '.data'
section names specifically, and I guess there's likely targets that
don't use those names.  I've limited the tests to GNU/Linux systems,
but maybe I need to be even stricter?

Anyway, the following is a description of why I think this patch is
correct.  The updated patch can be found at the end of the email.

With a focus on the user_defined_section_attribute variable, here's
how things currently work:

1. The variable user_defined_section_attribute is initialised to false
   at compile time.  This is done in toplev.c

2. The only other place that user_defined_section_attribute is set to
   false is in the function rest_of_handle_final in final.c.  This is
   part of the final compiler optimisation pass.  The
   rest_of_handle_final function is called from the
   pass_final::execute method.

3. The user_defined_section_attribute is set to true in only one
   place, in handle_section_attribute in c-family/c-common.c.  Setting
   user_defined_section_attribute to true always happens when the
   handle_section_attribute function is called, unless the target does
   not support named sections.

4. The handle_section_attribute function is called whenever any
   function or data has the section attribute attached.

5. The attribute handling function handle_section_attribute is called
   with the following call-stack (as an example):
   1) handle_section_attribute
   2) decl_attributes
   3) c_decl_attributes
   4) start_decl
   5) c_parser_declaration
   6) c_parser_external_declaration
   7) c_parser_translation_unit
   8) c_parse_file
   9) c_common_parse_file
   10) compile_file
   11) do_compile
   12) toplev::main
   13) main
   The middle levels 2 to 8 could change depending on where the
   section attribute is encountered, but the interesting thing to take
   away is that calls to handle_section_attribute originate from a
   call to c_common_parse_file, which is called from do_compile.
   Right now I believe that this is always the case, this is crucial
   to the validity of this patch, if I've got this wrong then this
   patch is wrong.

6. Specifically, within the compile_file function (in toplev.c) it is
   the line 'lang_hooks.parse_file ();' which leads to the attribute
   handling functions being called.

7. The user_defined_section_attribute is checked in only one place,
   this is in the method pass_partition_blocks::gate, in the file
   bb-reorder.c.  Like the reset to false in final.c (mentioned above)
   this checking of user_defined_section_attribute is part of a
   compiler optimisation pass.

8. Like the file parsing, the compiler optimisation passes originate
   from a call in compile_file (in toplev.c), this call occurs after
   the call to parse the file (obviously).

The problem here that I see then is that we first parse the whole C
file, handling all attributes, we then perform the optimisation passes
on all functions.  As user_defined_section_attribute starts as false
and is set true during the C parsing whenever a section attribute is
encountered, the variable will be true by the end of the parse process
if there is any function or variable with a section attribute.

We then perform the partition-blocks pass on the first function, which
will be disabled (if user_defined_section_attribute) is true,
regardless of whether it was actually that function that has a section
attribute attached.

We then perform the final pass on the first function at which point we
reset the user_defined_section_attribute to false.

When we perform the partition-blocks pass on the second function we
will see user_defined_section_attribute set to false regardless of
whether the function has a section attribute or not (the attribute
handling functions are only called during file parse, not during the
optimisation passes).

There is another issue, that if a variable has a section attribute
this will also cause user_defined_section_attribute to be set to true,
(which makes sense based on the name 'user_defined_section_attribute')
however, given what user_defined_section_attribute is actually used
for, there's no reason to disable the partition-blocks pass just
because some variable is assigned to a specific section.

In the revised patch I disable the partition-blocks pass for a
function only when the function DECL has a section attribute
attached.  This information is already available on the function DECL,
so there's no need for us to keep a separate variable around, and so I
delete user_defined_section_attribute.

I've added three new tests in this latest revision of the patch.
These cover issues that I claim to fix; a section attribute on an
earlier function prevents a later function being partitioned, and a
section attribute on a variable stops a function being partitioned
(there's two variants of this one).

In all of the test cases GCC fails to partition a function that could
have been partitioned.  We will never partition a function that should
not be partitioned, though it is possible to craft a test file where
the partition blocks pass is run on a function that should NOT be
partitioned (due to a section attribute) however, in that case the
section attribute causes the function to placed in the specified
section thanks to the section selection code in varasm.c

I don't currently have GCC write access.  If the patch is approved,
then please could someone apply it.

Thanks

Andrew

---

gcc: remove unneeded global related to hot/cold partitioning

The `user_defined_section_attribute' is used as part of the condition to
determine if GCC should partition blocks within a function into hot and
cold blocks.  This global is initially false, and is set to true from
within the file parse phase of GCC, as part of the attribute handling
hook.

The `user_defined_section_attribute' is reset to false as part of the
final pass of GCC.  However, the final pass is part of the optimisation
phase of the compiler, and so if at any point during the file parse
phase any function, or data, has a section attribute the global
`user_defined_section_attribute' will be set to true.

When GCC performs the block partitioning pass on the first function, if
`user_defined_section_attribute' is true then the function will not be
partitioned.  Notice though, that due to the above, whether we partition
this first function or not has nothing to do with whether the function
has a section attribute, instead, if any function or data in the parsed
file has a section attribute then we don't partition the first
function.

After performing (or not) the block partitioning pass on the first
function we perform the final pass on the first function, at which point
we reset `user_defined_section_attribute' to false.  As parsing is
complete by this point, we will never set
`user_defined_section_attribute' to true after that, and so all of the
following functions will have the partition blocks pass performed on
them, even if the function has a section attribute, and will not be
partitioned.

Luckily we don't end up partitioning functions that should not be
partitioned though.  Due to the way that functions are selected during
the assembler writing phase, if a function has a section attribute this
takes priority over any hot/cold block partitioning that has been done.

What we see from the above then is that the
`user_defined_section_attribute' mechanism is broken.  It was originally
created when GCC parsed, optimised, and generated assembler function at
a time.  Now that we deal with the whole file in one go, we need to
update the mechanism used to gate the block partitioning pass.

This patch does this by looking specifically for a section attribute on
the function DECL, which removes the need for a global variable, and
will work whether we parse the whole file in one go, or one function at
a time.

A few new tests have been added.  These check for the case where a
function is not partitioned when it could be.

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/testsuiteChangeLog:

	* gcc.dg/tree-prof/section-attr-1.c: New file.
	* gcc.dg/tree-prof/section-attr-2.c: New file.
	* gcc.dg/tree-prof/section-attr-3.c: New file.
---
 gcc/bb-reorder.c                                |  3 +-
 gcc/c-family/c-common.c                         |  2 --
 gcc/final.c                                     |  2 --
 gcc/testsuite/ChangeLog                         |  6 ++++
 gcc/testsuite/gcc.dg/tree-prof/section-attr-1.c | 43 +++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/tree-prof/section-attr-2.c | 43 +++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/tree-prof/section-attr-3.c | 43 +++++++++++++++++++++++++
 gcc/toplev.c                                    |  5 ---
 gcc/toplev.h                                    |  5 ---
 9 files changed, 136 insertions(+), 16 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-prof/section-attr-1.c
 create mode 100644 gcc/testsuite/gcc.dg/tree-prof/section-attr-2.c
 create mode 100644 gcc/testsuite/gcc.dg/tree-prof/section-attr-3.c

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

Commit Message

Comments

Patch