Message ID | 777a73110d5119e9fd4443d7e67798c7270edeba.1525775371.git.segher@kernel.crashing.org |
---|---|
State | New |
Headers | show |
Series | PR85645 | expand |
> Now, neither of the two branches needs to have LR restored at all, > because both of the branches end up in an infinite loop. > > This patch makes spread_component return a boolean saying if anything > was changed, and if so, it is called again. This obviously is finite > (there is a finite number of basic blocks, each with a finite number > of components, and spread_components can only assign more components > to a block, never less). I also instrumented the code, and on a > bootstrap+regtest spread_components made changes a maximum of two > times. Interestingly though it made changes on two iterations in > a third of the cases it did anything at all! I don't know the code much so I don't see why this solves the problem. > 2018-05-08 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/85645 > * shrink-wrap.c (spread_components): Return a boolean saying if > anything was changed. > (try_shrink_wrapping_separate): Iterate spread_components until > nothing changes anymore. OK if you add a comment in try_shrink_wrapping_separate with the rationale.
On Wed, May 09, 2018 at 09:33:30AM +0200, Eric Botcazou wrote: > > Now, neither of the two branches needs to have LR restored at all, > > because both of the branches end up in an infinite loop. > > > > This patch makes spread_component return a boolean saying if anything > > was changed, and if so, it is called again. This obviously is finite > > (there is a finite number of basic blocks, each with a finite number > > of components, and spread_components can only assign more components > > to a block, never less). I also instrumented the code, and on a > > bootstrap+regtest spread_components made changes a maximum of two > > times. Interestingly though it made changes on two iterations in > > a third of the cases it did anything at all! > > I don't know the code much so I don't see why this solves the problem. When I wrote the code I thought it would reach the fixpoint after just one iteration. This is not true though, not e.g. in the motivating example with no-return paths through the function. It didn't generate wrong code btw, just pretty silly^W^Wnot terribly good code. > > 2018-05-08 Segher Boessenkool <segher@kernel.crashing.org> > > > > PR rtl-optimization/85645 > > * shrink-wrap.c (spread_components): Return a boolean saying if > > anything was changed. > > (try_shrink_wrapping_separate): Iterate spread_components until > > nothing changes anymore. > > OK if you add a comment in try_shrink_wrapping_separate with the rationale. I made it say this: /* Try to minimize the number of saves and restores. Do this as long as it changes anything. This does not iterate more than a few times. */ int spread_times = 0; while (spread_components (components)) { spread_times++; if (dump_file) fprintf (dump_file, "Now spread %d times.\n", spread_times); } Thanks for the quick reviews, Segher
diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c index 6b47d4e..40b26b8 100644 --- a/gcc/shrink-wrap.c +++ b/gcc/shrink-wrap.c @@ -1253,8 +1253,9 @@ place_prologue_for_one_component (unsigned int which, basic_block head) /* Set HAS_COMPONENTS in every block to the maximum it can be set to without setting it on any path from entry to exit where it was not already set somewhere (or, for blocks that have no path to the exit, consider only - paths from the entry to the block itself). */ -static void + paths from the entry to the block itself). Return whether any changes + were made to some HAS_COMPONENTS. */ +static bool spread_components (sbitmap components) { basic_block entry_block = ENTRY_BLOCK_PTR_FOR_FN (cfun); @@ -1377,12 +1378,19 @@ spread_components (sbitmap components) /* Finally, mark everything not not needed both forwards and backwards. */ + bool did_changes = false; + FOR_EACH_BB_FN (bb, cfun) { + bitmap_copy (old, SW (bb)->has_components); + bitmap_and (SW (bb)->head_components, SW (bb)->head_components, SW (bb)->tail_components); bitmap_and_compl (SW (bb)->has_components, components, SW (bb)->head_components); + + if (!did_changes && !bitmap_equal_p (old, SW (bb)->has_components)) + did_changes = true; } FOR_ALL_BB_FN (bb, cfun) @@ -1394,6 +1402,8 @@ spread_components (sbitmap components) fprintf (dump_file, "\n"); } } + + return did_changes; } /* If we cannot handle placing some component's prologues or epilogues where @@ -1797,7 +1807,14 @@ try_shrink_wrapping_separate (basic_block first_bb) EXECUTE_IF_SET_IN_BITMAP (components, 0, j, sbi) place_prologue_for_one_component (j, first_bb); - spread_components (components); + int spread_times = 0; + while (spread_components (components)) + { + spread_times++; + + if (dump_file) + fprintf (dump_file, "Now spread %d times.\n", spread_times); + } disqualify_problematic_components (components);