* [PATCH, PR81030] Call compute_outgoing_frequencies at expand
@ 2017-07-17 11:22 Tom de Vries
2017-07-17 17:37 ` Jan Hubicka
2017-07-20 11:40 ` Jan Hubicka
0 siblings, 2 replies; 3+ messages in thread
From: Tom de Vries @ 2017-07-17 11:22 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 4556 bytes --]
Hi,
this patch fixes PR81030, an P1 ICE in expand.
I.
For the test-case from the patch compiled at -O1 we have at .optimized:
...
_21 = 0;
_22 = c.5_6 != 0;
_23 = _21 | _22;
if (_23 != 0)
goto <bb 7>; [73.27%] [count: INV]
else
goto <bb 6>; [26.73%] [count: INV]
;; succ: 7 [73.3% (guessed)] (TRUE_VALUE,EXECUTABLE)
;; 6 [26.7% (guessed)] (FALSE_VALUE,EXECUTABLE)
...
While expanding this bit, we first swap the operands of the BIT_IOR_EXPR:
...
;; Generating RTL for gimple basic block 4
Swap operands in stmt:
_23 = _21 | _22;
Cost left opnd=0, right opnd=2
...
And then in expand_gimple_cond we substitute the def of _23 and replace
the BIT_IOR_EXPR with TRUTH_ORIF_EXPR. So we really expand:
...
if (_22 || _21)
...
In do_jump_1, we encounter the following TRUTH_ORIF_EXPR handling:
...
case TRUTH_ORIF_EXPR:
{
/* Spread the probability evenly between the two conditions. So
the first condition has half the total probability of being true.
The second condition has the other half of the total probability,
so its jump has a probability of half the total, relative to
the probability we reached it (i.e. the first condition was
false). */
profile_probability op0_prob = profile_probability::uninitialized ();
profile_probability op1_prob = profile_probability::uninitialized ();
if (prob.initialized_p ())
{
op0_prob = prob.apply_scale (1, 2);
op1_prob = prob.apply_scale (1, 2) / op0_prob.invert ();
}
if (if_true_label == NULL)
{
drop_through_label = gen_label_rtx ();
do_jump (op0, NULL, drop_through_label, op0_prob);
do_jump (op1, if_false_label, NULL, op1_prob);
}
else
{
do_jump (op0, NULL, if_true_label, op0_prob);
do_jump (op1, if_false_label, if_true_label, op1_prob);
}
break;
}
...
This expands into two jumps. The intention is two conditional jumps, but
since _21 is 0, the second becomes unconditional:
...
;; if (_23 != 0)
(insn 12 11 13 4 (set (reg:CCZ 17 flags)
(compare:CCZ (mem/c:SI (symbol_ref:DI ("c") [flags 0x2]
<var_decl 0x7f0983db0f30 c>) [1 cD.1821+0 S4 A32])
(const_int 0 [0]))) "test2.c":20 -1
(nil))
(insn 13 12 14 4 (set (reg:QI 95)
(ne:QI (reg:CCZ 17 flags)
(const_int 0 [0]))) "test2.c":20 -1
(nil))
(insn 14 13 15 4 (set (reg:CCZ 17 flags)
(compare:CCZ (reg:QI 95)
(const_int 0 [0]))) "test2.c":20 -1
(nil))
(jump_insn 15 14 18 4 (set (pc)
(if_then_else (ne (reg:CCZ 17 flags)
(const_int 0 [0]))
(label_ref 0)
(pc))) "test2.c":20 -1
(int_list:REG_BR_PROB 3664 (nil)))
(note 18 15 16 10 [bb 10] NOTE_INSN_BASIC_BLOCK)
(jump_insn 16 18 17 10 (set (pc)
(label_ref 0)) -1
(nil))
(barrier 17 16 0)
...
The jump_insn 15 now contains a reg-note with the branch probability set
to 3664.
Eventually, in pass_expand::execute we call 'cleanup_cfg
(CLEANUP_NO_INSN_DEL)' during which we call checking_verify_flow_info,
which complains that:
...
test2.c:26:1: error: verify_flow_info: REG_BR_PROB does not match cfg
3664 7327
}
^
during RTL pass: expand
test2.c:26:1: internal compiler error: verify_flow_info failed
...
In other words, this jump_insn has a branch probability note of 3664,
but the corresponding edge claims a probability of 7327:
...
(jump_insn 15 14 18 5 (set (pc)
(if_then_else (ne (reg:CCZ 17 flags)
(const_int 0 [0]))
(label_ref:DI 32)
(pc))) "test2.c":20 617 {*jcc_1}
(int_list:REG_BR_PROB 3664 (nil))
-> 32)
;; succ: 10 [73.3% (guessed)]
;; 6 [26.7% (guessed)] (FALLTHRU)
...
II.
The ICE was introduced with r248945, by adding this bit to
find_many_sub_basic_blocks:
...
}
+ else
+ /* If nothing changed, there is no need to create new BBs. */
+ if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
+ continue;
compute_outgoing_frequencies (bb);
}
...
Commenting out this bit makes the compilation succeed.
III.
The bit added in find_many_sub_basic_blocks makes sense to me.
It just seems to me that we have been relying on
find_many_sub_basic_blocks to call compute_outgoing_frequencies for all
basic blocks during expand. This patch restores that situation, and
fixes the ICE.
Bootstrapped and reg-tested on x86_64.
OK for trunk?
Thanks,
- Tom
[-- Attachment #2: 0001-Call-compute_outgoing_frequencies-at-expand.patch --]
[-- Type: text/x-patch, Size: 2953 bytes --]
Call compute_outgoing_frequencies at expand
2017-07-17 Tom de Vries <tom@codesourcery.com>
PR middle-end/81030
* cfgbuild.c (find_many_sub_basic_blocks): Add and handle update_freq
parameter.
* cfgbuild.h (find_many_sub_basic_blocks): Add update_freq parameter.
* cfgexpand.c (pass_expand::execute): Call find_many_sub_basic_blocks
with update_freq == true.
* gcc.dg/pr81030.c: New test.
---
gcc/cfgbuild.c | 4 ++--
gcc/cfgbuild.h | 3 ++-
gcc/cfgexpand.c | 2 +-
gcc/testsuite/gcc.dg/pr81030.c | 29 +++++++++++++++++++++++++++++
4 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/gcc/cfgbuild.c b/gcc/cfgbuild.c
index 56a2cb9..78036fe 100644
--- a/gcc/cfgbuild.c
+++ b/gcc/cfgbuild.c
@@ -594,7 +594,7 @@ compute_outgoing_frequencies (basic_block b)
and create edges. */
void
-find_many_sub_basic_blocks (sbitmap blocks)
+find_many_sub_basic_blocks (sbitmap blocks, bool update_freq)
{
basic_block bb, min, max;
bool found = false;
@@ -677,7 +677,7 @@ find_many_sub_basic_blocks (sbitmap blocks)
}
else
/* If nothing changed, there is no need to create new BBs. */
- if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
+ if (!update_freq && EDGE_COUNT (bb->succs) == n_succs[bb->index])
continue;
compute_outgoing_frequencies (bb);
diff --git a/gcc/cfgbuild.h b/gcc/cfgbuild.h
index afda5ac..0c58590 100644
--- a/gcc/cfgbuild.h
+++ b/gcc/cfgbuild.h
@@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see
extern bool inside_basic_block_p (const rtx_insn *);
extern bool control_flow_insn_p (const rtx_insn *);
extern void rtl_make_eh_edge (sbitmap, basic_block, rtx);
-extern void find_many_sub_basic_blocks (sbitmap);
+extern void find_many_sub_basic_blocks (sbitmap block,
+ bool update_freq = false);
#endif /* GCC_CFGBUILD_H */
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 3e1d24d..e030a86 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -6464,7 +6464,7 @@ pass_expand::execute (function *fun)
auto_sbitmap blocks (last_basic_block_for_fn (fun));
bitmap_ones (blocks);
- find_many_sub_basic_blocks (blocks);
+ find_many_sub_basic_blocks (blocks, true);
purge_all_dead_edges ();
expand_stack_alignment ();
diff --git a/gcc/testsuite/gcc.dg/pr81030.c b/gcc/testsuite/gcc.dg/pr81030.c
new file mode 100644
index 0000000..23da6e5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr81030.c
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+void __assert_fail (const char *, const char *, unsigned int, const char *);
+
+int a, b, c, d, e, f, h;
+unsigned char g;
+
+int main ()
+{
+ int i, *j = &b;
+ if (a)
+ {
+ if (h)
+ {
+ int **k = &j;
+ d = 0;
+ *k = &e;
+ }
+ else
+ for (b = 0; b > -28; b = g)
+ ;
+ c || !j ? : __assert_fail ("c || !j", "small.c", 20, "main");
+ if (f)
+ for (i = 0; i < 1; i++)
+ ;
+ }
+ return 0;
+}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH, PR81030] Call compute_outgoing_frequencies at expand
2017-07-17 11:22 [PATCH, PR81030] Call compute_outgoing_frequencies at expand Tom de Vries
@ 2017-07-17 17:37 ` Jan Hubicka
2017-07-20 11:40 ` Jan Hubicka
1 sibling, 0 replies; 3+ messages in thread
From: Jan Hubicka @ 2017-07-17 17:37 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches
Hi,
thanks for looking into this issue. It is quite weird indeed.
> Commenting out this bit makes the compilation succeed.
so my understanding is that RTL expansions confuses itself and redistributes
probabilities to two jumps while immediately optimizing out the second...
I think in such case instead of calling compute_outgoing_frequencies which
will take confused probability from REG_BR_PROB_NOTE and produce inconsistent
profile, it is better to take the existing probabilities in profile and put
them back to RTL.
Of course it would be nice to teach RTl expansion to not do such mistakes
(because the patch bellow helps only in case the BB happens to not split at
all), but that would be quite difficult I guess.
I am testing the attached patch.
Honza
Index: cfgbuild.c
===================================================================
--- cfgbuild.c (revision 250246)
+++ cfgbuild.c (working copy)
@@ -676,7 +676,14 @@ find_many_sub_basic_blocks (sbitmap bloc
else
/* If nothing changed, there is no need to create new BBs. */
if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
- continue;
+ {
+ /* In rare occassions RTL expansion might have mistakely assigned
+ a probabilities different from what is in CFG. This happens
+ when we try to split branch to two but optimize out the
+ second branch during the way. See PR81030. */
+ update_br_prob_note (bb);
+ continue;
+ }
compute_outgoing_frequencies (bb);
}
>
>
> III.
>
> The bit added in find_many_sub_basic_blocks makes sense to me.
>
> It just seems to me that we have been relying on find_many_sub_basic_blocks
> to call compute_outgoing_frequencies for all basic blocks during expand.
> This patch restores that situation, and fixes the ICE.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk?
>
> Thanks,
> - Tom
> Call compute_outgoing_frequencies at expand
>
> 2017-07-17 Tom de Vries <tom@codesourcery.com>
>
> PR middle-end/81030
> * cfgbuild.c (find_many_sub_basic_blocks): Add and handle update_freq
> parameter.
> * cfgbuild.h (find_many_sub_basic_blocks): Add update_freq parameter.
> * cfgexpand.c (pass_expand::execute): Call find_many_sub_basic_blocks
> with update_freq == true.
>
> * gcc.dg/pr81030.c: New test.
>
> ---
> gcc/cfgbuild.c | 4 ++--
> gcc/cfgbuild.h | 3 ++-
> gcc/cfgexpand.c | 2 +-
> gcc/testsuite/gcc.dg/pr81030.c | 29 +++++++++++++++++++++++++++++
> 4 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/cfgbuild.c b/gcc/cfgbuild.c
> index 56a2cb9..78036fe 100644
> --- a/gcc/cfgbuild.c
> +++ b/gcc/cfgbuild.c
> @@ -594,7 +594,7 @@ compute_outgoing_frequencies (basic_block b)
> and create edges. */
>
> void
> -find_many_sub_basic_blocks (sbitmap blocks)
> +find_many_sub_basic_blocks (sbitmap blocks, bool update_freq)
> {
> basic_block bb, min, max;
> bool found = false;
> @@ -677,7 +677,7 @@ find_many_sub_basic_blocks (sbitmap blocks)
> }
> else
> /* If nothing changed, there is no need to create new BBs. */
> - if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
> + if (!update_freq && EDGE_COUNT (bb->succs) == n_succs[bb->index])
> continue;
>
> compute_outgoing_frequencies (bb);
> diff --git a/gcc/cfgbuild.h b/gcc/cfgbuild.h
> index afda5ac..0c58590 100644
> --- a/gcc/cfgbuild.h
> +++ b/gcc/cfgbuild.h
> @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see
> extern bool inside_basic_block_p (const rtx_insn *);
> extern bool control_flow_insn_p (const rtx_insn *);
> extern void rtl_make_eh_edge (sbitmap, basic_block, rtx);
> -extern void find_many_sub_basic_blocks (sbitmap);
> +extern void find_many_sub_basic_blocks (sbitmap block,
> + bool update_freq = false);
>
> #endif /* GCC_CFGBUILD_H */
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index 3e1d24d..e030a86 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -6464,7 +6464,7 @@ pass_expand::execute (function *fun)
>
> auto_sbitmap blocks (last_basic_block_for_fn (fun));
> bitmap_ones (blocks);
> - find_many_sub_basic_blocks (blocks);
> + find_many_sub_basic_blocks (blocks, true);
> purge_all_dead_edges ();
>
> expand_stack_alignment ();
> diff --git a/gcc/testsuite/gcc.dg/pr81030.c b/gcc/testsuite/gcc.dg/pr81030.c
> new file mode 100644
> index 0000000..23da6e5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr81030.c
> @@ -0,0 +1,29 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O1" } */
> +
> +void __assert_fail (const char *, const char *, unsigned int, const char *);
> +
> +int a, b, c, d, e, f, h;
> +unsigned char g;
> +
> +int main ()
> +{
> + int i, *j = &b;
> + if (a)
> + {
> + if (h)
> + {
> + int **k = &j;
> + d = 0;
> + *k = &e;
> + }
> + else
> + for (b = 0; b > -28; b = g)
> + ;
> + c || !j ? : __assert_fail ("c || !j", "small.c", 20, "main");
> + if (f)
> + for (i = 0; i < 1; i++)
> + ;
> + }
> + return 0;
> +}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH, PR81030] Call compute_outgoing_frequencies at expand
2017-07-17 11:22 [PATCH, PR81030] Call compute_outgoing_frequencies at expand Tom de Vries
2017-07-17 17:37 ` Jan Hubicka
@ 2017-07-20 11:40 ` Jan Hubicka
1 sibling, 0 replies; 3+ messages in thread
From: Jan Hubicka @ 2017-07-20 11:40 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches
Hi,
this is patch I comitted.
Thanks for looking into it,
Honza
PR middle-end/81030
* gcc.dg/pr81030.c: New test.
* cfgbuild.c (find_many_sub_basic_blocks): Update REG_BR_PROB note
when gimple level profile disagrees with what RTL expander did.
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog (revision 250378)
+++ testsuite/ChangeLog (working copy)
@@ -1,3 +1,8 @@
+2017-07-17 Tom de Vries <tom@codesourcery.com>
+
+ PR middle-end/81030
+ * gcc.dg/pr81030.c: New test.
+
2017-07-20 Naveen H.S <Naveen.Hurugalawadi@cavium.com>
* gcc.dg/tree-ssa/vrp116.c: New Test.
Index: testsuite/gcc.dg/pr81030.c
===================================================================
--- testsuite/gcc.dg/pr81030.c (revision 0)
+++ testsuite/gcc.dg/pr81030.c (working copy)
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+void __assert_fail (const char *, const char *, unsigned int, const char *);
+
+int a, b, c, d, e, f, h;
+unsigned char g;
+
+int main ()
+{
+ int i, *j = &b;
+ if (a)
+ {
+ if (h)
+ {
+ int **k = &j;
+ d = 0;
+ *k = &e;
+ }
+ else
+ for (b = 0; b > -28; b = g)
+ ;
+ c || !j ? : __assert_fail ("c || !j", "small.c", 20, "main");
+ if (f)
+ for (i = 0; i < 1; i++)
+ ;
+ }
+ return 0;
+}
Index: ChangeLog
===================================================================
--- ChangeLog (revision 250378)
+++ ChangeLog (working copy)
@@ -1,3 +1,10 @@
+2017-07-18 Jan Hubicka <hubicka@ucw.cz>
+ Tom de Vries <tom@codesourcery.com>
+
+ PR middle-end/81030
+ * cfgbuild.c (find_many_sub_basic_blocks): Update REG_BR_PROB note
+ when gimple level profile disagrees with what RTL expander did.
+
2017-07-20 Tom de Vries <tom@codesourcery.com>
PR tree-optimization/81489
Index: cfgbuild.c
===================================================================
--- cfgbuild.c (revision 250378)
+++ cfgbuild.c (working copy)
@@ -673,10 +673,18 @@ find_many_sub_basic_blocks (sbitmap bloc
&& profile_status_for_fn (cfun) != PROFILE_READ))
bb->count = profile_count::uninitialized ();
}
- else
- /* If nothing changed, there is no need to create new BBs. */
- if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
+ /* If nothing changed, there is no need to create new BBs. */
+ else if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
+ {
+ /* In rare occassions RTL expansion might have mistakely assigned
+ a probabilities different from what is in CFG. This happens
+ when we try to split branch to two but optimize out the
+ second branch during the way. See PR81030. */
+ if (JUMP_P (BB_END (bb)) && any_condjump_p (BB_END (bb))
+ && EDGE_COUNT (bb->succs) >= 2)
+ update_br_prob_note (bb);
continue;
+ }
compute_outgoing_frequencies (bb);
}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-07-20 11:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-17 11:22 [PATCH, PR81030] Call compute_outgoing_frequencies at expand Tom de Vries
2017-07-17 17:37 ` Jan Hubicka
2017-07-20 11:40 ` Jan Hubicka
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).