[PATCH 0/2] Remove user_defined_section_attribute global

[PATCH 0/2] Remove user_defined_section_attribute global

[Andrew Burgess]

The first patch contains the interesting change, the second is just a
small comment fix in a related area of code, that I spotted while
creating the first patch.

I don't have commit access, so if these are reviewed / approved,
please could they also be applied.

Thanks,
Andrew

---

Andrew Burgess (2):
  gcc: Remove unneeded global flag.
  gcc: Update comment in bb-reorder.c

 gcc/ChangeLog           | 16 ++++++++++++++++
 gcc/bb-reorder.c        |  6 ++----
 gcc/c-family/c-common.c |  2 --
 gcc/final.c             |  2 --
 gcc/toplev.c            |  5 -----
 gcc/toplev.h            |  5 -----
 6 files changed, 18 insertions(+), 18 deletions(-)

-- 
2.6.4

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

[Andrew Burgess]

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.
---
 gcc/ChangeLog           | 12 ++++++++++++
 gcc/bb-reorder.c        |  4 +---
 gcc/c-family/c-common.c |  2 --
 gcc/final.c             |  2 --
 gcc/toplev.c            |  5 -----
 gcc/toplev.h            |  5 -----
 6 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c
index 5fb60bd..04874da 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"
@@ -2889,8 +2888,7 @@ pass_partition_blocks::gate (function *fun)
 	  /* See gate_handle_reorder_blocks.  We should not partition if
 	     we are going to omit the reordering.  */
 	  && optimize_function_for_speed_p (fun)
-	  && !DECL_COMDAT_GROUP (current_function_decl)
-	  && !user_defined_section_attribute);
+	  && !DECL_COMDAT_GROUP (current_function_decl));
 }
 
 unsigned
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 94005ff..66add18 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -7617,8 +7617,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 543b8a3..868eecf 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.6.4

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

[Jeff Law]

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.

So what might be better to do here is introduce a test to verify proper 
behavior.

Jeff

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

[Jakub Jelinek]

On Tue, Jun 21, 2016 at 08:55:15PM -0600, Jeff Law wrote:
> 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.

The flag has been added before we had -funit-at-a-time, and IMNSHO it
couldn't work properly even when it has been introduced, because if you had
void foo (void) __attribute__((section ("...")));
void bar (void)
{
...
}
void foo (void)
{
...
}
then it would mistakenly apply to bar rather than foo.
So, either we can reconstruct whether the current function decl has user
section attribute, then perhaps during expansion we should set the flag
to that or use such a test directly in the gate (e.g. would lookup_attribute
("section", DECL_ATTRIBUTES (current_function_decl)) DTRT?) and drop the
flag, or we need some way to preserve that information per function.

	Jakub

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

[Andrew Burgess]

* 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

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

[Andrew Burgess]

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

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 1132a03..2d7107d 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -7751,8 +7751,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 eccc3d8..b407b85 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -4478,8 +4478,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/testsuite/gcc.dg/tree-prof/section-attr-1.c b/gcc/testsuite/gcc.dg/tree-prof/section-attr-1.c
new file mode 100644
index 0000000..51e38cf
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-prof/section-attr-1.c
@@ -0,0 +1,43 @@
+/* Checks for a bug where a function with a section attribute would prevent
+   all later functions from being partitioned into hot and cold blocks.  */
+/* { dg-require-effective-target freorder } */
+/* { dg-options "-O2 -fno-profile-reorder-functions -freorder-blocks-and-partition -save-temps" } */
+
+#define SIZE 10000
+
+const char *sarr[SIZE];
+const char *buf_hot;
+const char *buf_cold;
+
+void foo (int path);
+
+__attribute__((section(".text")))
+int
+main (int argc, char *argv[])
+{
+  int i;
+  buf_hot =  "hello";
+  buf_cold = "world";
+  for (i = 0; i < 1000000; i++)
+    foo (argc);
+  return 0;
+}
+
+__attribute__((noinline))
+void
+foo (int path)
+{
+  int i;
+  if (path)
+    {
+      for (i = 0; i < SIZE; i++)
+	sarr[i] = buf_hot;
+    }
+  else
+    {
+      for (i = 0; i < SIZE; i++)
+	sarr[i] = buf_cold;
+    }
+}
+
+/* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
diff --git a/gcc/testsuite/gcc.dg/tree-prof/section-attr-2.c b/gcc/testsuite/gcc.dg/tree-prof/section-attr-2.c
new file mode 100644
index 0000000..cbfa5fa
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-prof/section-attr-2.c
@@ -0,0 +1,43 @@
+/* Checks for a bug where static data with a section attribute within a
+   function would stop the function being partitioned into hot and cold
+   blocks.  */
+/* { dg-require-effective-target freorder } */
+/* { dg-options "-O2 -fno-profile-reorder-functions -freorder-blocks-and-partition -save-temps" } */
+
+#define SIZE 10000
+
+const char *sarr[SIZE];
+const char *buf_hot;
+const char *buf_cold;
+
+void foo (int path);
+
+int
+main (int argc, char *argv[])
+{
+  int i;
+  buf_hot =  "hello";
+  buf_cold = "world";
+  for (i = 0; i < 1000000; i++)
+    foo (argc);
+  return 0;
+}
+
+__attribute__((noinline))
+void
+foo (int path)
+{
+  static int i __attribute__((section(".data")));
+  if (path)
+    {
+      for (i = 0; i < SIZE; i++)
+	sarr[i] = buf_hot;
+    }
+  else
+    {
+      for (i = 0; i < SIZE; i++)
+	sarr[i] = buf_cold;
+    }
+}
+
+/* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
diff --git a/gcc/testsuite/gcc.dg/tree-prof/section-attr-3.c b/gcc/testsuite/gcc.dg/tree-prof/section-attr-3.c
new file mode 100644
index 0000000..593a94a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-prof/section-attr-3.c
@@ -0,0 +1,43 @@
+/* Checks for a bug where static data with a section attribute within a
+   function would stop the function being partitioned into hot and cold
+   blocks.  */
+/* { dg-require-effective-target freorder } */
+/* { dg-options "-O2 -fno-profile-reorder-functions -freorder-blocks-and-partition -save-temps" } */
+
+#define SIZE 10000
+
+const char *sarr[SIZE];
+const char *buf_hot __attribute__ ((section (".data")));
+const char *buf_cold;
+
+void foo (int path);
+
+int
+main (int argc, char *argv[])
+{
+  int i;
+  buf_hot =  "hello";
+  buf_cold = "world";
+  for (i = 0; i < 1000000; i++)
+    foo (argc);
+  return 0;
+}
+
+__attribute__((noinline))
+void
+foo (int path)
+{
+  int i;
+  if (path)
+    {
+      for (i = 0; i < SIZE; i++)
+	sarr[i] = buf_hot;
+    }
+  else
+    {
+      for (i = 0; i < SIZE; i++)
+	sarr[i] = buf_cold;
+    }
+}
+
+/* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 66099ec..113fc7f 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -149,11 +149,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.6.4





* Andrew Burgess <andrew.burgess@embecosm.com> [2016-06-29 20:21:30 +0100]:

> * 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
> 

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

[Jakub Jelinek]

On Wed, Sep 14, 2016 at 02:00:48PM +0100, Andrew Burgess wrote:
> 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 wonder if user_defined_section_attribute instead shouldn't be moved
into struct function and be handled as a per-function flag then.

	Jakub

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

[Andrew Burgess]

* Jakub Jelinek <jakub@redhat.com> [2016-09-14 15:07:56 +0200]:

> On Wed, Sep 14, 2016 at 02:00:48PM +0100, Andrew Burgess wrote:
> > 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 wonder if user_defined_section_attribute instead shouldn't be moved
> into struct function and be handled as a per-function flag then.

That would certainly solve the problem I'm trying to address.  But I
wonder, how is that different to looking for a section attribute on
the function DECL?

Thanks,
Andrew

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

[Jeff Law]

On 09/15/2016 08:24 AM, Andrew Burgess wrote:
> * Jakub Jelinek <jakub@redhat.com> [2016-09-14 15:07:56 +0200]:
>
>> On Wed, Sep 14, 2016 at 02:00:48PM +0100, Andrew Burgess wrote:
>>> 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 wonder if user_defined_section_attribute instead shouldn't be moved
>> into struct function and be handled as a per-function flag then.
>
> That would certainly solve the problem I'm trying to address.  But I
> wonder, how is that different to looking for a section attribute on
> the function DECL?
I'm not sure it is significantly different.  It seems like it's just an 
implementation detail.  I'd err on the side of putting this into the 
struct function rather than on the DECL node simply to keep the size of 
DECL nodes from increasing.  Even if you can find suitable free flag 
bits, those can likely be better used for other purposes.


I'm still pondering the actual patch.  It's not forgotten.

jeff

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

[Andrew Burgess]

* Jeff Law <law@redhat.com> [2016-10-28 09:58:14 -0600]:

> On 09/15/2016 08:24 AM, Andrew Burgess wrote:
> > * Jakub Jelinek <jakub@redhat.com> [2016-09-14 15:07:56 +0200]:
> > 
> > > On Wed, Sep 14, 2016 at 02:00:48PM +0100, Andrew Burgess wrote:
> > > > 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 wonder if user_defined_section_attribute instead shouldn't be moved
> > > into struct function and be handled as a per-function flag then.
> > 
> > That would certainly solve the problem I'm trying to address.  But I
> > wonder, how is that different to looking for a section attribute on
> > the function DECL?
> I'm not sure it is significantly different.  It seems like it's just an
> implementation detail.  I'd err on the side of putting this into the struct
> function rather than on the DECL node simply to keep the size of DECL nodes
> from increasing.  Even if you can find suitable free flag bits, those can
> likely be better used for other purposes.

I didn't add anything to the DECL, the information we need is already
there.  The relevant chunk of the patch is:

@@ -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

I have not made any changes to add anything new to the DECL.  I guess
an argument _could_ be made that looking up an attribute is too
expensive to be used in a pass::gate function (I haven't looked into
how expensive it is) but I figured that initially at least it's better
to reuse the data we already have around than to add a new flag that
duplicates something we already have.

> I'm still pondering the actual patch.  It's not forgotten.

Would it help clarify things if I added some printf style tracing and
posted a trace?  This might help highlight how
USER_DEFINED_SECTION_ATTRIBUTE is set in a different phase of
compilation and so can't possibly be of any use when deciding whether
or not to perform the pass or not.

I'm still keen to see this merged, so any extra leg work I can do to
help move this forward, please let me know; I'm happy to help.

Thanks,
Andrew

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

[Bernd Schmidt]

On 09/14/2016 03:00 PM, Andrew Burgess wrote:
> 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.
> 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.

I think the explanation is perfectly reasonable and the patch looks 
good, except:

> +__attribute__((noinline))

Add noclone to all of these as well.


Bernd

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

[Andrew Burgess]

* Bernd Schmidt <bschmidt@redhat.com> [2016-11-03 13:01:32 +0100]:

> On 09/14/2016 03:00 PM, Andrew Burgess wrote:
> > 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.
> > 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.
> 
> I think the explanation is perfectly reasonable and the patch looks good,
> except:
> 
> > +__attribute__((noinline))
> 
> Add noclone to all of these as well.

Thanks.  Considering Jeff said, I'm thinking about it, and you've said
yes, and given Jeff's not got back, I'm considering this patch
approved (with the fix you suggest).

My only remaining concern is the new tests, I've tried to restrict
them to targets that I suspect they'll pass on with:

    /* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */

but I'm still nervous that I'm going to introduce test failures.  Is
there any advice / guidance I should follow before I commit, or are
folk pretty relaxed so long as I've made a reasonable effort?

Thanks,
Andrew

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

[Mike Stump]

On Nov 16, 2016, at 12:09 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> My only remaining concern is the new tests, I've tried to restrict
> them to targets that I suspect they'll pass on with:
> 
>    /* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
> 
> but I'm still nervous that I'm going to introduce test failures.  Is
> there any advice / guidance I should follow before I commit, or are
> folk pretty relaxed so long as I've made a reasonable effort?

So, if you are worried about the way the line is constructed, I usually test it by misspelling the *-*-linux* *-*-gnu* part as *-*-linNOTux* *-*-gnNOTu* and see if the test then doesn't run on your machine.  If it doesn't then you can be pretty confident that only machines that match the target triplet can be impacted.  I usually do this type of testing by running the test case in isolation (not the full tests suite).  Anyway, do the best you can, and don't worry about t it too much, learn from the experience, even if it goes wrong in some way.  If it did go wrong, just be responsive (don't check it in just before a 6 week vacation) about fixing it, if you can.

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

[Andrew Burgess]

* Mike Stump <mikestump@comcast.net> [2016-11-16 12:59:53 -0800]:

> On Nov 16, 2016, at 12:09 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> > My only remaining concern is the new tests, I've tried to restrict
> > them to targets that I suspect they'll pass on with:
> > 
> >    /* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
> > 
> > but I'm still nervous that I'm going to introduce test failures.  Is
> > there any advice / guidance I should follow before I commit, or are
> > folk pretty relaxed so long as I've made a reasonable effort?
> 
> So, if you are worried about the way the line is constructed, I usually test it by misspelling the *-*-linux* *-*-gnu* part as *-*-linNOTux* *-*-gnNOTu* and see if the test then doesn't run on your machine.  If it doesn't then you can be pretty confident that only machines that match the target triplet can be impacted.  I usually do this type of testing by running the test case in isolation (not the full tests suite).  Anyway, do the best you can, and don't worry about t it too much, learn from the experience, even if it goes wrong in some way.  If it did go wrong, just be responsive (don't check it in just before a 6 week vacation) about fixing it, if you can.
> 

Thanks for the feedback.

Change committed as revision 242519.  If anyone sees any issues just
let me know.

Thanks,
Andrew

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

[Jeff Law]

On 11/16/2016 03:12 PM, Andrew Burgess wrote:
> * Mike Stump <mikestump@comcast.net> [2016-11-16 12:59:53 -0800]:
>
>> On Nov 16, 2016, at 12:09 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
>>> My only remaining concern is the new tests, I've tried to restrict
>>> them to targets that I suspect they'll pass on with:
>>>
>>>    /* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
>>>
>>> but I'm still nervous that I'm going to introduce test failures.  Is
>>> there any advice / guidance I should follow before I commit, or are
>>> folk pretty relaxed so long as I've made a reasonable effort?
>>
>> So, if you are worried about the way the line is constructed, I usually test it by misspelling the *-*-linux* *-*-gnu* part as *-*-linNOTux* *-*-gnNOTu* and see if the test then doesn't run on your machine.  If it doesn't then you can be pretty confident that only machines that match the target triplet can be impacted.  I usually do this type of testing by running the test case in isolation (not the full tests suite).  Anyway, do the best you can, and don't worry about t it too much, learn from the experience, even if it goes wrong in some way.  If it did go wrong, just be responsive (don't check it in just before a 6 week vacation) about fixing it, if you can.
>>
>
> Thanks for the feedback.
>
> Change committed as revision 242519.  If anyone sees any issues just
> let me know.
Thanks.  And thanks for seeing this through... I know it's a pain sometimes.

jeff

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

[Christophe Lyon]

On 16 November 2016 at 23:12, Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> * Mike Stump <mikestump@comcast.net> [2016-11-16 12:59:53 -0800]:
>
>> On Nov 16, 2016, at 12:09 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
>> > My only remaining concern is the new tests, I've tried to restrict
>> > them to targets that I suspect they'll pass on with:
>> >
>> >    /* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
>> >
>> > but I'm still nervous that I'm going to introduce test failures.  Is
>> > there any advice / guidance I should follow before I commit, or are
>> > folk pretty relaxed so long as I've made a reasonable effort?
>>
>> So, if you are worried about the way the line is constructed, I usually test it by misspelling the *-*-linux* *-*-gnu* part as *-*-linNOTux* *-*-gnNOTu* and see if the test then doesn't run on your machine.  If it doesn't then you can be pretty confident that only machines that match the target triplet can be impacted.  I usually do this type of testing by running the test case in isolation (not the full tests suite).  Anyway, do the best you can, and don't worry about t it too much, learn from the experience, even if it goes wrong in some way.  If it did go wrong, just be responsive (don't check it in just before a 6 week vacation) about fixing it, if you can.
>>
>
> Thanks for the feedback.
>
> Change committed as revision 242519.  If anyone sees any issues just
> let me know.
>

Hi Andrew,

Sorry for the delay, there are so many commits these days, with so
many regression
reports to check manually before reporting here....

So, your new test fails on arm* targets:

  gcc.dg/tree-prof/section-attr-1.c scan-assembler .section[\t
]*.text.unlikely[\\n\\r]+[\t ]*.size[\t ]*foo.cold.0
  gcc.dg/tree-prof/section-attr-2.c scan-assembler .section[\t
]*.text.unlikely[\\n\\r]+[\t ]*.size[\t ]*foo.cold.0
  gcc.dg/tree-prof/section-attr-3.c scan-assembler .section[\t
]*.text.unlikely[\\n\\r]+[\t ]*.size[\t ]*foo.cold.0

Christophe



> Thanks,
> Andrew

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

[Andrew Burgess]

* Christophe Lyon <christophe.lyon@linaro.org> [2016-11-18 13:21:50 +0100]:

> On 16 November 2016 at 23:12, Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > * Mike Stump <mikestump@comcast.net> [2016-11-16 12:59:53 -0800]:
> >
> >> On Nov 16, 2016, at 12:09 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> >> > My only remaining concern is the new tests, I've tried to restrict
> >> > them to targets that I suspect they'll pass on with:
> >> >
> >> >    /* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
> >> >
> >> > but I'm still nervous that I'm going to introduce test failures.  Is
> >> > there any advice / guidance I should follow before I commit, or are
> >> > folk pretty relaxed so long as I've made a reasonable effort?
> >>
> >> So, if you are worried about the way the line is constructed, I usually test it by misspelling the *-*-linux* *-*-gnu* part as *-*-linNOTux* *-*-gnNOTu* and see if the test then doesn't run on your machine.  If it doesn't then you can be pretty confident that only machines that match the target triplet can be impacted.  I usually do this type of testing by running the test case in isolation (not the full tests suite).  Anyway, do the best you can, and don't worry about t it too much, learn from the experience, even if it goes wrong in some way.  If it did go wrong, just be responsive (don't check it in just before a 6 week vacation) about fixing it, if you can.
> >>
> >
> > Thanks for the feedback.
> >
> > Change committed as revision 242519.  If anyone sees any issues just
> > let me know.
> >
> 
> Hi Andrew,
> 
> Sorry for the delay, there are so many commits these days, with so
> many regression
> reports to check manually before reporting here....
> 
> So, your new test fails on arm* targets:

After a little digging I think the problem might be that
-freorder-blocks-and-partition is not supported on arm.

This should be detected as the new tests include:

    /* { dg-require-effective-target freorder } */

however this test passed on arm as -freorder-blocks-and-partition does
not issue any warning unless -fprofile-use is also passed.

The patch below extends check_effective_target_freorder to check using
-fprofile-use.  With this change in place the tests are skipped on
arm.

All feedback welcome,

Thanks,
Andrew

---

arm/gcc: Tighten checks in check_effective_target_freorder

In check_effective_target_freorder we check to see if the target
 supports -freorder-blocks-and-partition.  However we disable
 -freorder-blocks-and-partition when -fprofile-use is not supplied so
 for some targets we'll not see any message about lack of support for
 -freorder-blocks-and-partition unless -fprofile-use is also passed.

This commit extends check_effective_target_freorder to first try
-freorder-blocks-and-partition on its own, then try -fprofile-use and
-freorder-blocks-and-partition.

gcc/testsuite/ChangeLog:

	* lib/target-supports.exp (check_effective_target_freorder): Check
	additional case.
---
 gcc/testsuite/ChangeLog               | 5 +++++
 gcc/testsuite/lib/target-supports.exp | 8 +++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 8a2abd2..0716792 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -1014,9 +1014,15 @@ proc check_effective_target_fstack_protector {} {
 # for trivial code, 0 otherwise.
 
 proc check_effective_target_freorder {} {
-    return [check_no_compiler_messages freorder object {
+    if { [check_no_compiler_messages freorder object {
 	void foo (void) { }
     } "-freorder-blocks-and-partition"]
+    && [check_no_compiler_messages fprofile_use_freorder object {
+	void foo (void) { }
+    } "-fprofile-use -freorder-blocks-and-partition"] } {
+	return 1
+    }
+    return 0
 }
 
 # Return 1 if -fpic and -fPIC are supported, as in no warnings or errors
-- 
2.6.4

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

[Mike Stump]

On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
>> So, your new test fails on arm* targets:
> 
> After a little digging I think the problem might be that
> -freorder-blocks-and-partition is not supported on arm.
> 
> This should be detected as the new tests include:
> 
>    /* { dg-require-effective-target freorder } */
> 
> however this test passed on arm as -freorder-blocks-and-partition does
> not issue any warning unless -fprofile-use is also passed.
> 
> The patch below extends check_effective_target_freorder to check using
> -fprofile-use.  With this change in place the tests are skipped on
> arm.

> All feedback welcome,

Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person thinks this is the wrong solution.

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

[Christophe Lyon]

On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote:
> On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
>>> So, your new test fails on arm* targets:
>>
>> After a little digging I think the problem might be that
>> -freorder-blocks-and-partition is not supported on arm.
>>
>> This should be detected as the new tests include:
>>
>>    /* { dg-require-effective-target freorder } */
>>
>> however this test passed on arm as -freorder-blocks-and-partition does
>> not issue any warning unless -fprofile-use is also passed.
>>
>> The patch below extends check_effective_target_freorder to check using
>> -fprofile-use.  With this change in place the tests are skipped on
>> arm.
>
>> All feedback welcome,
>
> Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person thinks this is the wrong solution.
>

Hi,

As promised, I tested this patch: it makes
gcc.dg/tree-prof/section-attr-[123].c
unsupported on arm*, and thus they are not failing anymore :-)

However, it also makes other tests unsupported, while they used to pass:

  gcc.dg/pr33648.c
  gcc.dg/pr46685.c
  gcc.dg/tree-prof/20041218-1.c
  gcc.dg/tree-prof/bb-reorg.c
  gcc.dg/tree-prof/cold_partition_label.c
  gcc.dg/tree-prof/comp-goto-1.c
  gcc.dg/tree-prof/pr34999.c
  gcc.dg/tree-prof/pr45354.c
  gcc.dg/tree-prof/pr50907.c
  gcc.dg/tree-prof/pr52027.c
  gcc.dg/tree-prof/va-arg-pack-1.c

and failures are now unsupported:
  gcc.dg/tree-prof/cold_partition_label.c
  gcc.dg/tree-prof/section-attr-1.c
  gcc.dg/tree-prof/section-attr-2.c
  gcc.dg/tree-prof/section-attr-3.c

So, maybe this patch is too strong?

Christophe

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

[Andrew Burgess]

* Christophe Lyon <christophe.lyon@linaro.org> [2016-11-21 13:47:09 +0100]:

> On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote:
> > On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> >>> So, your new test fails on arm* targets:
> >>
> >> After a little digging I think the problem might be that
> >> -freorder-blocks-and-partition is not supported on arm.
> >>
> >> This should be detected as the new tests include:
> >>
> >>    /* { dg-require-effective-target freorder } */
> >>
> >> however this test passed on arm as -freorder-blocks-and-partition does
> >> not issue any warning unless -fprofile-use is also passed.
> >>
> >> The patch below extends check_effective_target_freorder to check using
> >> -fprofile-use.  With this change in place the tests are skipped on
> >> arm.
> >
> >> All feedback welcome,
> >
> > Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person thinks this is the wrong solution.
> >
> 
> Hi,
> 
> As promised, I tested this patch: it makes
> gcc.dg/tree-prof/section-attr-[123].c
> unsupported on arm*, and thus they are not failing anymore :-)
> 
> However, it also makes other tests unsupported, while they used to pass:
> 
>   gcc.dg/pr33648.c
>   gcc.dg/pr46685.c
>   gcc.dg/tree-prof/20041218-1.c
>   gcc.dg/tree-prof/bb-reorg.c
>   gcc.dg/tree-prof/cold_partition_label.c
>   gcc.dg/tree-prof/comp-goto-1.c
>   gcc.dg/tree-prof/pr34999.c
>   gcc.dg/tree-prof/pr45354.c
>   gcc.dg/tree-prof/pr50907.c
>   gcc.dg/tree-prof/pr52027.c
>   gcc.dg/tree-prof/va-arg-pack-1.c
> 
> and failures are now unsupported:
>   gcc.dg/tree-prof/cold_partition_label.c
>   gcc.dg/tree-prof/section-attr-1.c
>   gcc.dg/tree-prof/section-attr-2.c
>   gcc.dg/tree-prof/section-attr-3.c
> 
> So, maybe this patch is too strong?

In all of the cases that used to pass the tests are compile only tests
(except for cold_partition_label, which I discuss below).

On ARM passing -fprofile-use and -freorder-blocks-and-partition
results in a warning, and the -freorder-blocks-and-partition flag is
ignored.  However, disabling -freorder-blocks-and-partition doesn't
stop any of the tests compiling, hence the passes.

All the tests include:

  /* { dg-require-effective-target freorder } */

which I understand to mean, the tests requires the 'freorder' feature
to be supported (which corresponds to -freorder-blocks-and-partition).

For cold_partition_label and my new tests it's seems clear that the
lack of support for -freorder-blocks-and-partition on ARM is the cause
of the test failures.

So, is it reasonable to give up the other tests as "unsupported"?  I'd
be inclined to say yes, but I happy to rework the patch if anyone has
a suggestion for an alternative approach.

One possibility would be to split the 'freorder' feature into two, one
being 'Can -freorder-blocks-and-partition be used without causing an
error (so ARM would say yes to this)', and a new feature would be
'Does -freorder-blocks-and-partition actually _work_ on the target?'
we'd then use this in cold_partition_label and in my new tests.
Though this doesn't feel a great solution....

Thanks,
Andrew

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

[Jeff Law]

On 11/24/2016 02:40 PM, Andrew Burgess wrote:
> * Christophe Lyon <christophe.lyon@linaro.org> [2016-11-21 13:47:09 +0100]:
>
>> On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote:
>>> On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
>>>>> So, your new test fails on arm* targets:
>>>>
>>>> After a little digging I think the problem might be that
>>>> -freorder-blocks-and-partition is not supported on arm.
>>>>
>>>> This should be detected as the new tests include:
>>>>
>>>>    /* { dg-require-effective-target freorder } */
>>>>
>>>> however this test passed on arm as -freorder-blocks-and-partition does
>>>> not issue any warning unless -fprofile-use is also passed.
>>>>
>>>> The patch below extends check_effective_target_freorder to check using
>>>> -fprofile-use.  With this change in place the tests are skipped on
>>>> arm.
>>>
>>>> All feedback welcome,
>>>
>>> Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person thinks this is the wrong solution.
>>>
>>
>> Hi,
>>
>> As promised, I tested this patch: it makes
>> gcc.dg/tree-prof/section-attr-[123].c
>> unsupported on arm*, and thus they are not failing anymore :-)
>>
>> However, it also makes other tests unsupported, while they used to pass:
>>
>>   gcc.dg/pr33648.c
>>   gcc.dg/pr46685.c
>>   gcc.dg/tree-prof/20041218-1.c
>>   gcc.dg/tree-prof/bb-reorg.c
>>   gcc.dg/tree-prof/cold_partition_label.c
>>   gcc.dg/tree-prof/comp-goto-1.c
>>   gcc.dg/tree-prof/pr34999.c
>>   gcc.dg/tree-prof/pr45354.c
>>   gcc.dg/tree-prof/pr50907.c
>>   gcc.dg/tree-prof/pr52027.c
>>   gcc.dg/tree-prof/va-arg-pack-1.c
>>
>> and failures are now unsupported:
>>   gcc.dg/tree-prof/cold_partition_label.c
>>   gcc.dg/tree-prof/section-attr-1.c
>>   gcc.dg/tree-prof/section-attr-2.c
>>   gcc.dg/tree-prof/section-attr-3.c
>>
>> So, maybe this patch is too strong?
>
> In all of the cases that used to pass the tests are compile only tests
> (except for cold_partition_label, which I discuss below).
>
> On ARM passing -fprofile-use and -freorder-blocks-and-partition
> results in a warning, and the -freorder-blocks-and-partition flag is
> ignored.  However, disabling -freorder-blocks-and-partition doesn't
> stop any of the tests compiling, hence the passes.
>
> All the tests include:
>
>   /* { dg-require-effective-target freorder } */
>
> which I understand to mean, the tests requires the 'freorder' feature
> to be supported (which corresponds to -freorder-blocks-and-partition).
>
> For cold_partition_label and my new tests it's seems clear that the
> lack of support for -freorder-blocks-and-partition on ARM is the cause
> of the test failures.
>
> So, is it reasonable to give up the other tests as "unsupported"?  I'd
> be inclined to say yes, but I happy to rework the patch if anyone has
> a suggestion for an alternative approach.
It is reasonable.  It's not uncommon to have to drop various tests to 
UNSUPPORTED, particularly things which depend on assembler/linker 
capabilities, the target runtime system, etc.

jeff

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

[Andrew Burgess]

* Jeff Law <law@redhat.com> [2016-11-28 15:08:46 -0700]:

> On 11/24/2016 02:40 PM, Andrew Burgess wrote:
> > * Christophe Lyon <christophe.lyon@linaro.org> [2016-11-21 13:47:09 +0100]:
> > 
> > > On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote:
> > > > On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> > > > > > So, your new test fails on arm* targets:
> > > > > 
> > > > > After a little digging I think the problem might be that
> > > > > -freorder-blocks-and-partition is not supported on arm.
> > > > > 
> > > > > This should be detected as the new tests include:
> > > > > 
> > > > >    /* { dg-require-effective-target freorder } */
> > > > > 
> > > > > however this test passed on arm as -freorder-blocks-and-partition does
> > > > > not issue any warning unless -fprofile-use is also passed.
> > > > > 
> > > > > The patch below extends check_effective_target_freorder to check using
> > > > > -fprofile-use.  With this change in place the tests are skipped on
> > > > > arm.
> > > > 
> > > > > All feedback welcome,
> > > > 
> > > > Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person thinks this is the wrong solution.
> > > > 
> > > 
> > > Hi,
> > > 
> > > As promised, I tested this patch: it makes
> > > gcc.dg/tree-prof/section-attr-[123].c
> > > unsupported on arm*, and thus they are not failing anymore :-)
> > > 
> > > However, it also makes other tests unsupported, while they used to pass:
> > > 
> > >   gcc.dg/pr33648.c
> > >   gcc.dg/pr46685.c
> > >   gcc.dg/tree-prof/20041218-1.c
> > >   gcc.dg/tree-prof/bb-reorg.c
> > >   gcc.dg/tree-prof/cold_partition_label.c
> > >   gcc.dg/tree-prof/comp-goto-1.c
> > >   gcc.dg/tree-prof/pr34999.c
> > >   gcc.dg/tree-prof/pr45354.c
> > >   gcc.dg/tree-prof/pr50907.c
> > >   gcc.dg/tree-prof/pr52027.c
> > >   gcc.dg/tree-prof/va-arg-pack-1.c
> > > 
> > > and failures are now unsupported:
> > >   gcc.dg/tree-prof/cold_partition_label.c
> > >   gcc.dg/tree-prof/section-attr-1.c
> > >   gcc.dg/tree-prof/section-attr-2.c
> > >   gcc.dg/tree-prof/section-attr-3.c
> > > 
> > > So, maybe this patch is too strong?
> > 
> > In all of the cases that used to pass the tests are compile only tests
> > (except for cold_partition_label, which I discuss below).
> > 
> > On ARM passing -fprofile-use and -freorder-blocks-and-partition
> > results in a warning, and the -freorder-blocks-and-partition flag is
> > ignored.  However, disabling -freorder-blocks-and-partition doesn't
> > stop any of the tests compiling, hence the passes.
> > 
> > All the tests include:
> > 
> >   /* { dg-require-effective-target freorder } */
> > 
> > which I understand to mean, the tests requires the 'freorder' feature
> > to be supported (which corresponds to -freorder-blocks-and-partition).
> > 
> > For cold_partition_label and my new tests it's seems clear that the
> > lack of support for -freorder-blocks-and-partition on ARM is the cause
> > of the test failures.
> > 
> > So, is it reasonable to give up the other tests as "unsupported"?  I'd
> > be inclined to say yes, but I happy to rework the patch if anyone has
> > a suggestion for an alternative approach.
> It is reasonable.  It's not uncommon to have to drop various tests to
> UNSUPPORTED, particularly things which depend on assembler/linker
> capabilities, the target runtime system, etc.

OK, I'm going to take that as approval for my patch[1].  I'll wait a
couple of days to give people a chance to correct me, then I'll push
the change.  This should resolve the test regressions I introduced for
ARM.

Thanks,
Andrew

[1] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02050.html

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

[Jeff Law]

On 11/29/2016 07:02 AM, Andrew Burgess wrote:
> * Jeff Law <law@redhat.com> [2016-11-28 15:08:46 -0700]:
>
>> On 11/24/2016 02:40 PM, Andrew Burgess wrote:
>>> * Christophe Lyon <christophe.lyon@linaro.org> [2016-11-21 13:47:09 +0100]:
>>>
>>>> On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote:
>>>>> On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
>>>>>>> So, your new test fails on arm* targets:
>>>>>>
>>>>>> After a little digging I think the problem might be that
>>>>>> -freorder-blocks-and-partition is not supported on arm.
>>>>>>
>>>>>> This should be detected as the new tests include:
>>>>>>
>>>>>>    /* { dg-require-effective-target freorder } */
>>>>>>
>>>>>> however this test passed on arm as -freorder-blocks-and-partition does
>>>>>> not issue any warning unless -fprofile-use is also passed.
>>>>>>
>>>>>> The patch below extends check_effective_target_freorder to check using
>>>>>> -fprofile-use.  With this change in place the tests are skipped on
>>>>>> arm.
>>>>>
>>>>>> All feedback welcome,
>>>>>
>>>>> Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person thinks this is the wrong solution.
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> As promised, I tested this patch: it makes
>>>> gcc.dg/tree-prof/section-attr-[123].c
>>>> unsupported on arm*, and thus they are not failing anymore :-)
>>>>
>>>> However, it also makes other tests unsupported, while they used to pass:
>>>>
>>>>   gcc.dg/pr33648.c
>>>>   gcc.dg/pr46685.c
>>>>   gcc.dg/tree-prof/20041218-1.c
>>>>   gcc.dg/tree-prof/bb-reorg.c
>>>>   gcc.dg/tree-prof/cold_partition_label.c
>>>>   gcc.dg/tree-prof/comp-goto-1.c
>>>>   gcc.dg/tree-prof/pr34999.c
>>>>   gcc.dg/tree-prof/pr45354.c
>>>>   gcc.dg/tree-prof/pr50907.c
>>>>   gcc.dg/tree-prof/pr52027.c
>>>>   gcc.dg/tree-prof/va-arg-pack-1.c
>>>>
>>>> and failures are now unsupported:
>>>>   gcc.dg/tree-prof/cold_partition_label.c
>>>>   gcc.dg/tree-prof/section-attr-1.c
>>>>   gcc.dg/tree-prof/section-attr-2.c
>>>>   gcc.dg/tree-prof/section-attr-3.c
>>>>
>>>> So, maybe this patch is too strong?
>>>
>>> In all of the cases that used to pass the tests are compile only tests
>>> (except for cold_partition_label, which I discuss below).
>>>
>>> On ARM passing -fprofile-use and -freorder-blocks-and-partition
>>> results in a warning, and the -freorder-blocks-and-partition flag is
>>> ignored.  However, disabling -freorder-blocks-and-partition doesn't
>>> stop any of the tests compiling, hence the passes.
>>>
>>> All the tests include:
>>>
>>>   /* { dg-require-effective-target freorder } */
>>>
>>> which I understand to mean, the tests requires the 'freorder' feature
>>> to be supported (which corresponds to -freorder-blocks-and-partition).
>>>
>>> For cold_partition_label and my new tests it's seems clear that the
>>> lack of support for -freorder-blocks-and-partition on ARM is the cause
>>> of the test failures.
>>>
>>> So, is it reasonable to give up the other tests as "unsupported"?  I'd
>>> be inclined to say yes, but I happy to rework the patch if anyone has
>>> a suggestion for an alternative approach.
>> It is reasonable.  It's not uncommon to have to drop various tests to
>> UNSUPPORTED, particularly things which depend on assembler/linker
>> capabilities, the target runtime system, etc.
>
> OK, I'm going to take that as approval for my patch[1].  I'll wait a
> couple of days to give people a chance to correct me, then I'll push
> the change.  This should resolve the test regressions I introduced for
> ARM.
I'll just go ahead and explicitly ACK this.

Thanks,
jeff

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

[Andrew Burgess]

* Jeff Law <law@redhat.com> [2016-11-29 10:35:50 -0700]:

> On 11/29/2016 07:02 AM, Andrew Burgess wrote:
> > * Jeff Law <law@redhat.com> [2016-11-28 15:08:46 -0700]:
> > 
> > > On 11/24/2016 02:40 PM, Andrew Burgess wrote:
> > > > * Christophe Lyon <christophe.lyon@linaro.org> [2016-11-21 13:47:09 +0100]:
> > > > 
> > > > > On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote:
> > > > > > On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> > > > > > > > So, your new test fails on arm* targets:
> > > > > > > 
> > > > > > > After a little digging I think the problem might be that
> > > > > > > -freorder-blocks-and-partition is not supported on arm.
> > > > > > > 
> > > > > > > This should be detected as the new tests include:
> > > > > > > 
> > > > > > >    /* { dg-require-effective-target freorder } */
> > > > > > > 
> > > > > > > however this test passed on arm as -freorder-blocks-and-partition does
> > > > > > > not issue any warning unless -fprofile-use is also passed.
> > > > > > > 
> > > > > > > The patch below extends check_effective_target_freorder to check using
> > > > > > > -fprofile-use.  With this change in place the tests are skipped on
> > > > > > > arm.
> > > > > > 
> > > > > > > All feedback welcome,
> > > > > > 
> > > > > > Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person thinks this is the wrong solution.
> > > > > > 
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > As promised, I tested this patch: it makes
> > > > > gcc.dg/tree-prof/section-attr-[123].c
> > > > > unsupported on arm*, and thus they are not failing anymore :-)
> > > > > 
> > > > > However, it also makes other tests unsupported, while they used to pass:
> > > > > 
> > > > >   gcc.dg/pr33648.c
> > > > >   gcc.dg/pr46685.c
> > > > >   gcc.dg/tree-prof/20041218-1.c
> > > > >   gcc.dg/tree-prof/bb-reorg.c
> > > > >   gcc.dg/tree-prof/cold_partition_label.c
> > > > >   gcc.dg/tree-prof/comp-goto-1.c
> > > > >   gcc.dg/tree-prof/pr34999.c
> > > > >   gcc.dg/tree-prof/pr45354.c
> > > > >   gcc.dg/tree-prof/pr50907.c
> > > > >   gcc.dg/tree-prof/pr52027.c
> > > > >   gcc.dg/tree-prof/va-arg-pack-1.c
> > > > > 
> > > > > and failures are now unsupported:
> > > > >   gcc.dg/tree-prof/cold_partition_label.c
> > > > >   gcc.dg/tree-prof/section-attr-1.c
> > > > >   gcc.dg/tree-prof/section-attr-2.c
> > > > >   gcc.dg/tree-prof/section-attr-3.c
> > > > > 
> > > > > So, maybe this patch is too strong?
> > > > 
> > > > In all of the cases that used to pass the tests are compile only tests
> > > > (except for cold_partition_label, which I discuss below).
> > > > 
> > > > On ARM passing -fprofile-use and -freorder-blocks-and-partition
> > > > results in a warning, and the -freorder-blocks-and-partition flag is
> > > > ignored.  However, disabling -freorder-blocks-and-partition doesn't
> > > > stop any of the tests compiling, hence the passes.
> > > > 
> > > > All the tests include:
> > > > 
> > > >   /* { dg-require-effective-target freorder } */
> > > > 
> > > > which I understand to mean, the tests requires the 'freorder' feature
> > > > to be supported (which corresponds to -freorder-blocks-and-partition).
> > > > 
> > > > For cold_partition_label and my new tests it's seems clear that the
> > > > lack of support for -freorder-blocks-and-partition on ARM is the cause
> > > > of the test failures.
> > > > 
> > > > So, is it reasonable to give up the other tests as "unsupported"?  I'd
> > > > be inclined to say yes, but I happy to rework the patch if anyone has
> > > > a suggestion for an alternative approach.
> > > It is reasonable.  It's not uncommon to have to drop various tests to
> > > UNSUPPORTED, particularly things which depend on assembler/linker
> > > capabilities, the target runtime system, etc.
> > 
> > OK, I'm going to take that as approval for my patch[1].  I'll wait a
> > couple of days to give people a chance to correct me, then I'll push
> > the change.  This should resolve the test regressions I introduced for
> > ARM.
> I'll just go ahead and explicitly ACK this.

Committed as r243009.

Thanks,
Andrew

[PATCH 2/2] gcc: Update comment in bb-reorder.c

[Andrew Burgess]

Out of date comment ... fixed.

gcc/ChangeLog:

	* gcc/bb-reorder.c (pass_partition_blocks::gate): Update comment.
---
 gcc/ChangeLog    | 4 ++++
 gcc/bb-reorder.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c
index 04874da..78d15e2 100644
--- a/gcc/bb-reorder.c
+++ b/gcc/bb-reorder.c
@@ -2885,7 +2885,7 @@ pass_partition_blocks::gate (function *fun)
      arises.  */
   return (flag_reorder_blocks_and_partition
 	  && optimize
-	  /* See gate_handle_reorder_blocks.  We should not partition if
+	  /* See pass_reorder_blocks::gate.  We should not partition if
 	     we are going to omit the reordering.  */
 	  && optimize_function_for_speed_p (fun)
 	  && !DECL_COMDAT_GROUP (current_function_decl));
-- 
2.6.4

Re: [PATCH 2/2] gcc: Update comment in bb-reorder.c

[Jeff Law]

On 06/10/2016 10:56 AM, Andrew Burgess wrote:
> 	* gcc/bb-reorder.c (pass_partition_blocks::gate): Update comment.
Thanks.  Installed.

jeff