* [PATCH, PR68715] Add missing single_pred_p test in scop_detection::merge_sese
@ 2016-03-16 7:58 Tom de Vries
2016-03-16 8:53 ` Richard Biener
0 siblings, 1 reply; 3+ messages in thread
From: Tom de Vries @ 2016-03-16 7:58 UTC (permalink / raw)
To: Richard Biener, Sebastian Pop; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]
Hi,
this patch fixes graphite PR68715, a 6 regression.
In scop_detection::merge_sese, we check if the exit bb of the merged
sese region is dominated by the entry bb:
...
if (...
|| !dominated_by_p (CDI_DOMINATORS, get_exit_bb (combined),
get_entry_bb (combined)))
{
...
return invalid_sese;
}
...
Subsequently, we check for an empty exit bb, and if that one's not
empty, we try to merge an empty successor block into the sese:
...
/* FIXME: We should remove this piece of code once
canonicalize_loop_closed_ssa has been removed, because that
function adds a BB with single exit. */
if (!trivially_empty_bb_p (get_exit_bb (combined)))
{
/* Find the first empty succ (with single exit) of
combined.exit. */
basic_block imm_succ = combined.exit->dest;
if (single_succ_p (imm_succ)
&& trivially_empty_bb_p (imm_succ))
combined.exit = single_succ_edge (imm_succ);
else
{
...
return invalid_sese;
}
}
...
However, when the imm_succ block has more than one predecessor, merging
the block into the sese region breaks the property that the exit bb is
dominated by the entry bb. We then run into an ICE in
harmful_loop_in_region a bit later, when re-checking that property.
The patch fixes this by adding a test for 'single_pred_p (imm_succ)' in
the empty-block-merge condition.
Bootstrapped and reg-tested on x86_64.
OK for stage4 trunk?
Thanks,
- Tom
[-- Attachment #2: 0002-Add-missing-single_pred_p-test-in-scop_detection-merge_sese.patch --]
[-- Type: text/x-patch, Size: 3626 bytes --]
Add missing single_pred_p test in scop_detection::merge_sese
2016-03-15 Tom de Vries <tom@codesourcery.com>
PR tree-optimization/68715
* graphite-scop-detection.c (scop_detection::merge_sese): Add missing
single_pred_p test.
* gcc.dg/graphite/pr68715-2.c: New test.
* gcc.dg/graphite/pr68715.c: New test.
* gfortran.dg/graphite/pr68715.f90: New test.
---
gcc/graphite-scop-detection.c | 4 ++-
gcc/testsuite/gcc.dg/graphite/pr68715-2.c | 35 +++++++++++++++++++++++++
gcc/testsuite/gcc.dg/graphite/pr68715.c | 36 ++++++++++++++++++++++++++
gcc/testsuite/gfortran.dg/graphite/pr68715.f90 | 31 ++++++++++++++++++++++
4 files changed, 105 insertions(+), 1 deletion(-)
diff --git a/gcc/graphite-scop-detection.c b/gcc/graphite-scop-detection.c
index 9161cb7..f0c13ee 100644
--- a/gcc/graphite-scop-detection.c
+++ b/gcc/graphite-scop-detection.c
@@ -836,7 +836,9 @@ scop_detection::merge_sese (sese_l first, sese_l second) const
{
/* Find the first empty succ (with single exit) of combined.exit. */
basic_block imm_succ = combined.exit->dest;
- if (single_succ_p (imm_succ) && trivially_empty_bb_p (imm_succ))
+ if (single_succ_p (imm_succ)
+ && single_pred_p (imm_succ)
+ && trivially_empty_bb_p (imm_succ))
combined.exit = single_succ_edge (imm_succ);
else
{
diff --git a/gcc/testsuite/gcc.dg/graphite/pr68715-2.c b/gcc/testsuite/gcc.dg/graphite/pr68715-2.c
new file mode 100644
index 0000000..270d948
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/graphite/pr68715-2.c
@@ -0,0 +1,35 @@
+/* { dg-do compile } */
+/* { dg-options "-Ofast -floop-interchange" } */
+
+int a, b, c, d, f, g;
+int e[1], h[1];
+void fn2 ();
+void fn3 ();
+void
+fn1 ()
+{
+ fn2 ();
+ b = 0;
+ for (; b < 10; b++)
+ ;
+}
+
+void
+fn2 ()
+{
+ if (a)
+ {
+ fn3 ();
+ c = d;
+ }
+}
+
+void
+fn3 ()
+{
+ for (; g; g++)
+ e[g] = 2;
+ if (f)
+ for (; g; g++)
+ h[g] = 5;
+}
diff --git a/gcc/testsuite/gcc.dg/graphite/pr68715.c b/gcc/testsuite/gcc.dg/graphite/pr68715.c
new file mode 100644
index 0000000..14da2fb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/graphite/pr68715.c
@@ -0,0 +1,36 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -floop-interchange" } */
+
+int a[1], c[1];
+int b, d, e;
+
+void
+fn1 (int p1)
+{
+ for (;;)
+ ;
+}
+
+int
+fn3 ()
+{
+ for (; e; e++)
+ c[e] = 2;
+ for (; d; d--)
+ a[d] = 8;
+ return 0;
+}
+
+int fn5 (int);
+
+int
+fn2 ()
+{
+ fn3 ();
+}
+
+void
+fn4 ()
+{
+ fn1 (b || fn5 (fn2 ()));
+}
diff --git a/gcc/testsuite/gfortran.dg/graphite/pr68715.f90 b/gcc/testsuite/gfortran.dg/graphite/pr68715.f90
new file mode 100644
index 0000000..c011756
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/graphite/pr68715.f90
@@ -0,0 +1,31 @@
+! { dg-do compile }
+! { dg-options "-floop-nest-optimize -O1" }
+
+SUBROUTINE se_core_core_interaction(calculate_forces)
+ INTEGER, PARAMETER :: dp=8
+ LOGICAL, INTENT(in) :: calculate_forces
+ REAL(KIND=dp), DIMENSION(3) :: force_ab, rij
+ LOGICAL :: lfoo,kfoo,mfoo,nfoo,ffoo
+ INTEGER, PARAMETER :: mi2=42
+ CALL dummy(lfoo,kfoo,mfoo,nfoo,method_id,core_core)
+ IF (lfoo) THEN
+ DO WHILE (ffoo())
+ IF (lfoo) CYCLE
+ IF (kfoo) CYCLE
+ dr1 = DOT_PRODUCT(rij,rij)
+ IF (dr1 > rij_threshold) THEN
+ SELECT CASE (method_id)
+ CASE (mi2)
+ IF (calculate_forces) THEN
+ CALL dummy2(force_ab)
+ IF (nfoo) THEN
+ force_ab = force_ab + core_core*dr3inv
+ END IF
+ END IF
+ END SELECT
+ END IF
+ enuclear = enuclear + enucij
+ END DO
+ CALL dummy3(enuclear)
+ END IF
+END SUBROUTINE se_core_core_interaction
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH, PR68715] Add missing single_pred_p test in scop_detection::merge_sese
2016-03-16 7:58 [PATCH, PR68715] Add missing single_pred_p test in scop_detection::merge_sese Tom de Vries
@ 2016-03-16 8:53 ` Richard Biener
2016-03-16 10:16 ` Tom de Vries
0 siblings, 1 reply; 3+ messages in thread
From: Richard Biener @ 2016-03-16 8:53 UTC (permalink / raw)
To: Tom de Vries; +Cc: Sebastian Pop, GCC Patches
On Wed, 16 Mar 2016, Tom de Vries wrote:
> Hi,
>
> this patch fixes graphite PR68715, a 6 regression.
>
> In scop_detection::merge_sese, we check if the exit bb of the merged sese
> region is dominated by the entry bb:
> ...
> if (...
> || !dominated_by_p (CDI_DOMINATORS, get_exit_bb (combined),
> get_entry_bb (combined)))
> {
> ...
> return invalid_sese;
> }
> ...
>
> Subsequently, we check for an empty exit bb, and if that one's not empty, we
> try to merge an empty successor block into the sese:
> ...
> /* FIXME: We should remove this piece of code once
> canonicalize_loop_closed_ssa has been removed, because that
> function adds a BB with single exit. */
> if (!trivially_empty_bb_p (get_exit_bb (combined)))
> {
> /* Find the first empty succ (with single exit) of
> combined.exit. */
> basic_block imm_succ = combined.exit->dest;
> if (single_succ_p (imm_succ)
> && trivially_empty_bb_p (imm_succ))
> combined.exit = single_succ_edge (imm_succ);
> else
> {
> ...
> return invalid_sese;
> }
> }
> ...
>
> However, when the imm_succ block has more than one predecessor, merging the
> block into the sese region breaks the property that the exit bb is dominated
> by the entry bb. We then run into an ICE in harmful_loop_in_region a bit
> later, when re-checking that property.
>
> The patch fixes this by adding a test for 'single_pred_p (imm_succ)' in the
> empty-block-merge condition.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for stage4 trunk?
Hmm, it looks like for all what this function does this effectively
pessimizes scop merging and it would be easier to split 'exit'
in case its destination is unsuitable (not trivially empty).
The
/* For now we just want to bail out when exit does not post-dominate
entry.
TODO: We might just add a basic_block at the exit to make exit
post-dominate entry (the entire region). */
if (!dominated_by_p (CDI_POST_DOMINATORS, get_entry_bb (combined),
get_exit_bb (combined))
|| !dominated_by_p (CDI_DOMINATORS, get_exit_bb (combined),
comment also suggests that splitting the get_nearest_pdom_with_single_exit
edge and including the new BB in the combined region would also always fix
the dominance relation (though I don't see how it could do that and
the comment looks wrong and by construction the check should never
trigger).
Otherwise the patch is certainly fine of course.
Thanks,
Richard.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH, PR68715] Add missing single_pred_p test in scop_detection::merge_sese
2016-03-16 8:53 ` Richard Biener
@ 2016-03-16 10:16 ` Tom de Vries
0 siblings, 0 replies; 3+ messages in thread
From: Tom de Vries @ 2016-03-16 10:16 UTC (permalink / raw)
To: Richard Biener; +Cc: Sebastian Pop, GCC Patches
On 16/03/16 09:53, Richard Biener wrote:
> Hmm, it looks like for all what this function does this effectively
> pessimizes scop merging and it would be easier to split 'exit'
> in case its destination is unsuitable (not trivially empty).
>
Agreed.
> The
>
> /* For now we just want to bail out when exit does not post-dominate
> entry.
> TODO: We might just add a basic_block at the exit to make exit
> post-dominate entry (the entire region). */
> if (!dominated_by_p (CDI_POST_DOMINATORS, get_entry_bb (combined),
> get_exit_bb (combined))
> || !dominated_by_p (CDI_DOMINATORS, get_exit_bb (combined),
>
> comment also suggests that splitting the get_nearest_pdom_with_single_exit
> edge and including the new BB in the combined region would also always fix
> the dominance relation (though I don't see how it could do that and
> the comment looks wrong and by construction the check should never
> trigger).
>
I've replaced the entire condition with two asserts:
...
diff --git {a,b}/gcc/graphite-scop-detection.c
index 7615842..762a248 100644
--- a/gcc/graphite-scop-detection.c
+++ b/gcc/graphite-scop-detection.c
@@ -820,14 +820,10 @@ scop_detection::merge_sese
/* For now we just want to bail out when exit does not post-dominate
entry.
TODO: We might just add a basic_block at the exit to make exit
post-dominate entry (the entire region). */
- if (!dominated_by_p (CDI_POST_DOMINATORS, get_entry_bb (combined),
- get_exit_bb (combined))
- || !dominated_by_p (CDI_DOMINATORS, get_exit_bb (combined),
- get_entry_bb (combined)))
- {
- DEBUG_PRINT (dp << "[scop-detection-fail] cannot merge "
- "seses.\n");
- return invalid_sese;
- }
+ gcc_assert (dominated_by_p (CDI_POST_DOMINATORS,
+ get_entry_bb (combined),
+ get_exit_bb (combined)));
+ gcc_assert (dominated_by_p (CDI_DOMINATORS,
+ get_exit_bb (combined),
+ get_entry_bb (combined)));
/* FIXME: We should remove this piece of code once
canonicalize_loop_closed_ssa has been removed, because that function
...
and ran graphite.exp.
I ran into an ICE for pr68693.f90, for the second assert. The exit bb
has two incoming edges from within the sese, and one from outside. Given
that the exit bb is empty, we could split the exit edge and redirect the
edge from outside the sese to the new bb, which would fix the sese.
Perhaps that is what is meant in the comment, I'm not sure.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-16 10:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-16 7:58 [PATCH, PR68715] Add missing single_pred_p test in scop_detection::merge_sese Tom de Vries
2016-03-16 8:53 ` Richard Biener
2016-03-16 10:16 ` Tom de Vries
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).