Message ID | a1846b548ba9345fde9fab897271215dc03b6acf.1443028412.git.segher@kernel.crashing.org |
---|---|
State | New |
Headers | show |
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
diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c index 40e9e50..e09f344 100644 --- a/gcc/bb-reorder.c +++ b/gcc/bb-reorder.c @@ -2429,10 +2429,19 @@ reorder_basic_blocks (void) set_edge_can_fallthru_flag (); mark_dfs_back_edges (); - if (1) - reorder_basic_blocks_software_trace_cache (); - else - reorder_basic_blocks_simple (); + switch (flag_reorder_blocks_algorithm) + { + case REORDER_BLOCKS_ALGORITHM_SIMPLE: + reorder_basic_blocks_simple (); + break; + + case REORDER_BLOCKS_ALGORITHM_STC: + reorder_basic_blocks_software_trace_cache (); + break; + + default: + gcc_unreachable (); + } relink_block_chain (/*stay_in_cfglayout_mode=*/true); diff --git a/gcc/common.opt b/gcc/common.opt index 94d1d88..b0f70fb 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1910,6 +1910,19 @@ freorder-blocks Common Report Var(flag_reorder_blocks) Optimization Reorder basic blocks to improve code placement +freorder-blocks-algorithm= +Common Joined RejectNegative Enum(reorder_blocks_algorithm) Var(flag_reorder_blocks_algorithm) Init(REORDER_BLOCKS_ALGORITHM_SIMPLE) Optimization +-freorder-blocks-algorithm=[simple|stc] Set the used basic block reordering algorithm + +Enum +Name(reorder_blocks_algorithm) Type(enum reorder_blocks_algorithm) UnknownError(unknown basic block reordering algorithm %qs) + +EnumValue +Enum(reorder_blocks_algorithm) String(simple) Value(REORDER_BLOCKS_ALGORITHM_SIMPLE) + +EnumValue +Enum(reorder_blocks_algorithm) String(stc) Value(REORDER_BLOCKS_ALGORITHM_STC) + freorder-blocks-and-partition Common Report Var(flag_reorder_blocks_and_partition) Optimization Reorder basic blocks and partition into hot and cold sections diff --git a/gcc/flag-types.h b/gcc/flag-types.h index ac9ca0b..6301cea 100644 --- a/gcc/flag-types.h +++ b/gcc/flag-types.h @@ -109,6 +109,13 @@ enum stack_reuse_level SR_ALL }; +/* The algorithm used for basic block reordering. */ +enum reorder_blocks_algorithm +{ + REORDER_BLOCKS_ALGORITHM_SIMPLE, + REORDER_BLOCKS_ALGORITHM_STC +}; + /* The algorithm used for the integrated register allocator (IRA). */ enum ira_algorithm { diff --git a/gcc/opts.c b/gcc/opts.c index f1a9acd..786fd3a 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -441,6 +441,7 @@ static const struct default_options default_options_table[] = { OPT_LEVELS_1_PLUS, OPT_fipa_reference, NULL, 1 }, { OPT_LEVELS_1_PLUS, OPT_fipa_profile, NULL, 1 }, { OPT_LEVELS_1_PLUS, OPT_fmerge_constants, NULL, 1 }, + { OPT_LEVELS_1_PLUS, OPT_freorder_blocks, NULL, 1 }, { OPT_LEVELS_1_PLUS, OPT_fshrink_wrap, NULL, 1 }, { OPT_LEVELS_1_PLUS, OPT_fsplit_wide_types, NULL, 1 }, { OPT_LEVELS_1_PLUS, OPT_ftree_ccp, NULL, 1 }, @@ -483,7 +484,8 @@ static const struct default_options default_options_table[] = #endif { OPT_LEVELS_2_PLUS, OPT_fstrict_aliasing, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_fstrict_overflow, NULL, 1 }, - { OPT_LEVELS_2_PLUS, OPT_freorder_blocks, NULL, 1 }, + { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_freorder_blocks_algorithm_, NULL, + REORDER_BLOCKS_ALGORITHM_STC }, { OPT_LEVELS_2_PLUS, OPT_freorder_functions, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_ftree_vrp, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_ftree_builtin_call_dce, NULL, 1 },