[3/4] bb-reorder: Add -freorder-blocks-algorithm= and wire it up

This adds an -freorder-blocks-algorithm=[simple|stc] flag, with "simple"
as default.  For -O2 and up (except -Os) it is switched to "stc" instead.
Targets that never want STC can override this.  This changes -freorder-blocks
to be on at -O1 and up (was -O2 and up).

In effect, the changes are for -O1 (which now gets "simple" instead of
nothing), -Os (which now gets "simple" instead of "stc", since STC results
in much bigger code), and for targets that wish to never use STC (not in
this patch though).


2015-09-23   Segher Boessenkool  <segher@kernel.crashing.org>

	* bb-reorder.c (reorder_basic_blocks): Use the algorithm selected
	with flag_reorder_blocks_algorithm.
	* common.opt (freorder-blocks-algorithm=): New flag.
	(reorder_blocks_algorithm): New enum.
	* flag-types.h (reorder_blocks_algorithm): New enum.
	* opts.c (default_options_table): Use -freorder-blocks at -O1 and up,
	and -freorder-blocks-algorithm=stc at -O2 and up (not at -Os).

---
 gcc/bb-reorder.c | 17 +++++++++++++----
 gcc/common.opt   | 13 +++++++++++++
 gcc/flag-types.h |  7 +++++++
 gcc/opts.c       |  4 +++-
 4 files changed, 36 insertions(+), 5 deletions(-)

On 09/24/2015 12:06 AM, Segher Boessenkool wrote:
> This adds an -freorder-blocks-algorithm=[simple|stc] flag, with "simple"
> as default.  For -O2 and up (except -Os) it is switched to "stc" instead.
> Targets that never want STC can override this.  This changes -freorder-blocks
> to be on at -O1 and up (was -O2 and up).
>
> In effect, the changes are for -O1 (which now gets "simple" instead of
> nothing), -Os (which now gets "simple" instead of "stc", since STC results
> in much bigger code), and for targets that wish to never use STC (not in
> this patch though).

This should be merged with its documentation in 4/4, and personally I'd 
have no problem reviewing a patch with 2/3/4 all in one. Splitting 
patches is most helpful if there are parts that rearrange things such as 
your 1/4, or if there are multiple independent functional changes. I'm 
not saying you did anything wrong by splitting, just that maybe you made 
unnecessary work for yourself.

No objections to 3/4 and 4/4 otherwise.


Bernd

On Thu, Sep 24, 2015 at 12:28:08PM +0200, Bernd Schmidt wrote:
> On 09/24/2015 12:06 AM, Segher Boessenkool wrote:
> >This adds an -freorder-blocks-algorithm=[simple|stc] flag, with "simple"
> >as default.  For -O2 and up (except -Os) it is switched to "stc" instead.
> >Targets that never want STC can override this.  This changes 
> >-freorder-blocks
> >to be on at -O1 and up (was -O2 and up).
> >
> >In effect, the changes are for -O1 (which now gets "simple" instead of
> >nothing), -Os (which now gets "simple" instead of "stc", since STC results
> >in much bigger code), and for targets that wish to never use STC (not in
> >this patch though).
> 
> This should be merged with its documentation in 4/4, and personally I'd 
> have no problem reviewing a patch with 2/3/4 all in one. Splitting 
> patches is most helpful if there are parts that rearrange things such as 
> your 1/4, or if there are multiple independent functional changes. I'm 
> not saying you did anything wrong by splitting, just that maybe you made 
> unnecessary work for yourself.

I had the patches like that in my git tree, so I figured I'd send it like
that, makes review slightly easier (not a big deal for small patches like
this of course).  I did not waste time splitting things up, don't worry :-)


Segher

Segher Boessenkool <segher@kernel.crashing.org> writes:
>
> In effect, the changes are for -O1 (which now gets "simple" instead of
> nothing), -Os (which now gets "simple" instead of "stc", since STC results
> in much bigger code), and for targets that wish to never use STC (not in
> this patch though).

Do you have some data on the code size differences with -Os?

-Andi

On Thu, Sep 24, 2015 at 08:12:55AM -0700, Andi Kleen wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> >
> > In effect, the changes are for -O1 (which now gets "simple" instead of
> > nothing), -Os (which now gets "simple" instead of "stc", since STC results
> > in much bigger code), and for targets that wish to never use STC (not in
> > this patch though).
> 
> Do you have some data on the code size differences with -Os?

It's about 0.1% for a quick combine.ii sniff test; I don't have big test
results for -Os.

"Much bigger code" is a mischaracterisation, that is true for -O2, not -Os.


Segher

On 02/11/15 14:38, Alan Lawrence wrote:
 >
> I'm a bit puzzled as to why nobody else has been seeing this, as it's been
> happening to me as part of building gcc on x86_64, but since this patch I've
> been seeing an ICE in vec::operator[] in reorder_basic_blocks_simple, building
> libitm/beginend.cc. Preprocessed source attached

OK, so I realize now just how big that preprocessed source was (1.4MB) - 
apologies! I've filed PR/68182 with a .gz for now....

On Mon, Nov 02, 2015 at 02:38:33PM +0000, Alan Lawrence wrote:
> On 23/09/15 23:06, Segher Boessenkool wrote:
> >This adds an -freorder-blocks-algorithm=[simple|stc] flag, with "simple"
> >as default.  For -O2 and up (except -Os) it is switched to "stc" instead.
> >Targets that never want STC can override this.  This changes 
> >-freorder-blocks
> >to be on at -O1 and up (was -O2 and up).
> >
> >In effect, the changes are for -O1 (which now gets "simple" instead of
> >nothing), -Os (which now gets "simple" instead of "stc", since STC results
> >in much bigger code), and for targets that wish to never use STC (not in
> >this patch though).
> >
> >
> >2015-09-23   Segher Boessenkool  <segher@kernel.crashing.org>
> >
> >	* bb-reorder.c (reorder_basic_blocks): Use the algorithm selected
> >	with flag_reorder_blocks_algorithm.
> >	* common.opt (freorder-blocks-algorithm=): New flag.
> >	(reorder_blocks_algorithm): New enum.
> >	* flag-types.h (reorder_blocks_algorithm): New enum.
> >	* opts.c (default_options_table): Use -freorder-blocks at -O1 and up,
> >	and -freorder-blocks-algorithm=stc at -O2 and up (not at -Os).
> 
> I'm a bit puzzled as to why nobody else has been seeing this, as it's been 
> happening to me as part of building gcc on x86_64, but since this patch 
> I've been seeing an ICE in vec::operator[] in reorder_basic_blocks_simple, 
> building libitm/beginend.cc. Preprocessed source attached, command line 
> reduced to:
> 
> $ /work/alalaw01/build/./gcc/xg++ -B/work/alalaw01/build/./gcc/ -mrtm -O1 
> -g -m32 -c temp.ii
> /work/alalaw01/src/gcc/libitm/beginend.cc: In static member function 
> ‘static uint32_t GTM::gtm_thread::begin_transaction(uint32_t, const 
> gtm_jmpbuf*)’:
> /work/alalaw01/src/gcc/libitm/beginend.cc:400:1: internal compiler error: 
> in operator[], at vec.h:714
>  }
>  ^
> 0x1310783 vec<edge_def*, va_gc, vl_embed>::operator[](unsigned int)
> 	/work/alalaw01/src/gcc/gcc/vec.h:714
> 0x1310783 reorder_basic_blocks_simple
> 	/work/alalaw01/src/gcc/gcc/bb-reorder.c:2322

That seems to be happening in the FOR_EACH_BB_FN then?  Or one of the
EDGE_SUCCs?  I cannot match up that line # with anything.  Both cases
suggest things are broken before already.

> 0x1310783 reorder_basic_blocks
> 	/work/alalaw01/src/gcc/gcc/bb-reorder.c:2450
> 0x1310783 execute
> 	/work/alalaw01/src/gcc/gcc/bb-reorder.c:2551
> 
> Compilation succeeds at -O2 (instead of -O1).

reorder_basic_blocks_simple isn't used at -O2 (it uses STC instead).

I'll have a look at this wednesday.


Segher