tree-optimization/103188 - avoid running ranger on not-up-to-date SSA
Commit Message
The following splits loop header copying into an analysis phase
that uses ranger and a transform phase that can do without to avoid
running ranger on IL that has SSA form not updated.
Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.
2021-11-11 Richard Biener <rguenther@suse.de>
PR tree-optimization/103188
* tree-ssa-loop-ch.c (should_duplicate_loop_header_p):
Remove query parameter, split out check for size
optimization.
(ch_base::m_ranger, cb_base::m_query): Remove.
(ch_base::copy_headers): Split processing loop into
analysis around which we allocate and use ranger and
transform where we do not.
(pass_ch::execute): Do not allocate/free ranger here.
(pass_ch_vect::execute): Likewise.
* gcc.dg/torture/pr103188.c: New testcase.
---
gcc/testsuite/gcc.dg/torture/pr103188.c | 38 +++++++++++++
gcc/tree-ssa-loop-ch.c | 72 ++++++++++++++-----------
2 files changed, 78 insertions(+), 32 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/torture/pr103188.c
Comments
Thanks for doing this!
>
> + gimple_ranger *ranger = new gimple_ranger;
> + path_range_query *query = new path_range_query (*ranger, /*resolve=*/true);
Hmmm, it looks like both clients are now instantiating a gimple_ranger
just so they can pass it down to the path_range_query. Maybe we
should have another ctor with just:
path_range_query (bool resolve);
...and have it allocate its own ranger.
Does this seem like a useful improvement? For that matter, resolve
should default to true. The option is only there so the backward
threader can run in a "light" mode (early threading, etc).
Aldy
On November 11, 2021 5:43:48 PM GMT+01:00, Aldy Hernandez <aldyh@redhat.com> wrote:
>Thanks for doing this!
>
>>
>> + gimple_ranger *ranger = new gimple_ranger;
>> + path_range_query *query = new path_range_query (*ranger, /*resolve=*/true);
>
>Hmmm, it looks like both clients are now instantiating a gimple_ranger
>just so they can pass it down to the path_range_query. Maybe we
>should have another ctor with just:
>
>path_range_query (bool resolve);
>
>...and have it allocate its own ranger.
>
>Does this seem like a useful improvement? For that matter, resolve
>should default to true. The option is only there so the backward
>threader can run in a "light" mode (early threading, etc).
I've just copied from the two duplicate instances of this, so I don't know nothing here ;)
Richard.
>
>Aldy
>
Like this. It simplifies both loop-ch and the threader.
I'll push this pending tests unless someone objects.
On Thu, Nov 11, 2021 at 5:43 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> Thanks for doing this!
>
> >
> > + gimple_ranger *ranger = new gimple_ranger;
> > + path_range_query *query = new path_range_query (*ranger, /*resolve=*/true);
>
> Hmmm, it looks like both clients are now instantiating a gimple_ranger
> just so they can pass it down to the path_range_query. Maybe we
> should have another ctor with just:
>
> path_range_query (bool resolve);
>
> ...and have it allocate its own ranger.
>
> Does this seem like a useful improvement? For that matter, resolve
> should default to true. The option is only there so the backward
> threader can run in a "light" mode (early threading, etc).
>
> Aldy
new file mode 100644
@@ -0,0 +1,38 @@
+/* { dg-do compile } */
+
+int a, b, c, d = 10, e = 1, f, g, h, i;
+int main()
+{
+ int j = -1;
+k:
+ h = c;
+l:
+ c = ~c;
+ if (e)
+ m:
+ a = 0;
+ if (j > 1)
+ goto m;
+ if (!e)
+ goto l;
+ if (c)
+ goto p;
+n:
+ goto m;
+o:
+ if (f) {
+ if (g)
+ goto k;
+ j = 0;
+ p:
+ if (d)
+ goto o;
+ goto n;
+ }
+ if (i)
+ goto l;
+ for (; a < 1; a++)
+ while (a > d)
+ b++;
+ return 0;
+}
@@ -69,26 +69,12 @@ entry_loop_condition_is_static (class loop *l, path_range_query *query)
static bool
should_duplicate_loop_header_p (basic_block header, class loop *loop,
- int *limit, path_range_query *query)
+ int *limit)
{
gimple_stmt_iterator bsi;
gcc_assert (!header->aux);
- /* Avoid loop header copying when optimizing for size unless we can
- determine that the loop condition is static in the first
- iteration. */
- if (optimize_loop_for_size_p (loop)
- && !loop->force_vectorize
- && !entry_loop_condition_is_static (loop, query))
- {
- if (dump_file && (dump_flags & TDF_DETAILS))
- fprintf (dump_file,
- " Not duplicating bb %i: optimizing for size.\n",
- header->index);
- return false;
- }
-
gcc_assert (EDGE_COUNT (header->succs) > 0);
if (single_succ_p (header))
{
@@ -223,8 +209,6 @@ should_duplicate_loop_header_p (basic_block header, class loop *loop,
return false;
}
- if (dump_file && (dump_flags & TDF_DETAILS))
- fprintf (dump_file, " Will duplicate bb %i\n", header->index);
return true;
}
@@ -289,9 +273,6 @@ class ch_base : public gimple_opt_pass
/* Return true to copy headers of LOOP or false to skip. */
virtual bool process_loop_p (class loop *loop) = 0;
-
- gimple_ranger *m_ranger = NULL;
- path_range_query *m_query = NULL;
};
const pass_data pass_data_ch =
@@ -386,8 +367,11 @@ ch_base::copy_headers (function *fun)
copied_bbs = XNEWVEC (basic_block, n_basic_blocks_for_fn (fun));
bbs_size = n_basic_blocks_for_fn (fun);
+ auto_vec<loop_p> candidates;
auto_vec<std::pair<edge, loop_p> > copied;
+ gimple_ranger *ranger = new gimple_ranger;
+ path_range_query *query = new path_range_query (*ranger, /*resolve=*/true);
for (auto loop : loops_list (cfun, 0))
{
int initial_limit = param_max_loop_header_insns;
@@ -406,6 +390,37 @@ ch_base::copy_headers (function *fun)
|| !process_loop_p (loop))
continue;
+ /* Avoid loop header copying when optimizing for size unless we can
+ determine that the loop condition is static in the first
+ iteration. */
+ if (optimize_loop_for_size_p (loop)
+ && !loop->force_vectorize
+ && !entry_loop_condition_is_static (loop, query))
+ {
+ if (dump_file && (dump_flags & TDF_DETAILS))
+ fprintf (dump_file,
+ " Not duplicating bb %i: optimizing for size.\n",
+ header->index);
+ continue;
+ }
+
+ if (should_duplicate_loop_header_p (header, loop, &remaining_limit))
+ candidates.safe_push (loop);
+ }
+ /* Do not use ranger after we change the IL and not have updated SSA. */
+ delete query;
+ delete ranger;
+
+ for (auto loop : candidates)
+ {
+ int initial_limit = param_max_loop_header_insns;
+ int remaining_limit = initial_limit;
+ if (dump_file && (dump_flags & TDF_DETAILS))
+ fprintf (dump_file,
+ "Copying headers of loop %i\n", loop->num);
+
+ header = loop->header;
+
/* Iterate the header copying up to limit; this takes care of the cases
like while (a && b) {...}, where we want to have both of the conditions
copied. TODO -- handle while (a || b) - like cases, by not requiring
@@ -414,9 +429,11 @@ ch_base::copy_headers (function *fun)
exit = NULL;
n_bbs = 0;
- while (should_duplicate_loop_header_p (header, loop, &remaining_limit,
- m_query))
+ while (should_duplicate_loop_header_p (header, loop, &remaining_limit))
{
+ if (dump_file && (dump_flags & TDF_DETAILS))
+ fprintf (dump_file, " Will duplicate bb %i\n", header->index);
+
/* Find a successor of header that is inside a loop; i.e. the new
header after the condition is copied. */
if (flow_bb_inside_loop_p (loop, EDGE_SUCC (header, 0)->dest))
@@ -552,13 +569,9 @@ pass_ch::execute (function *fun)
loop_optimizer_init (LOOPS_HAVE_PREHEADERS
| LOOPS_HAVE_SIMPLE_LATCHES
| LOOPS_HAVE_RECORDED_EXITS);
- m_ranger = new gimple_ranger;
- m_query = new path_range_query (*m_ranger, /*resolve=*/true);
unsigned int res = copy_headers (fun);
- delete m_query;
- delete m_ranger;
loop_optimizer_finalize ();
return res;
}
@@ -570,12 +583,7 @@ pass_ch::execute (function *fun)
unsigned int
pass_ch_vect::execute (function *fun)
{
- m_ranger = new gimple_ranger;
- m_query = new path_range_query (*m_ranger, /*resolve=*/true);
- unsigned int res = copy_headers (fun);
- delete m_query;
- delete m_ranger;
- return res;
+ return copy_headers (fun);
}
/* Apply header copying according to a very simple test of do-while shape. */