Re: [PATCH] Verify edge probability consistency in verify_flow_info
> On 08/04/2017 11:15 AM, Jan Hubicka wrote: > >>>OK for trunk if bootstrap and reg-test on x86_64 succeeds? > >>Yea, but I'd like to see ongoing work towards full checking. > > > >I have full checking in my tree for some time. At x86-64 bootstrap there > >is one remaining offender in simd_clone_adjust which was not fixed yet > >https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00219.html > >Jakub did not tell me what would be a reasonable guess :) > > I think I just fixed that one here ( > https://gcc.gnu.org/ml/gcc-cvs/2017-08/msg00112.html ). Thanks! I will re-check then where we are standing wrt enabling checking everywhere ;) Honza > > Thanks, > - Tom
Re: [PATCH] Verify edge probability consistency in verify_flow_info
On 08/04/2017 11:15 AM, Jan Hubicka wrote:
III.
I've written this patch to check for the missing probability more
consistently. I'm not certain if we can require that the probability
should always be set, so I'm just requiring that if it is set on one
outgoing edge, it needs to be set on all outgoing edges.
Sofar I've build a c-only x86_64 non-bootstrap compiler and ran dg.exp.
The only problem I ran into was in attr-simd{,-2,-4}.c. I've written a
tentative patch for that, and will submit it as follow-up.
Is this check a good idea?
I think the additional checking is a good idea. Ideally we'd verify
that all edges have a probability. Until then I think you need some
kind of rationale in a comment for why the checking is limited.
[ Not done yet. ]
I've extended the patch to skip over EH edges. I've limited the check
further (as shown in attached patch) to only check if 'EDGE_COUNT
(bb->succs) == 2'. That way I don't run into the more complicated cases
like a switch with default edge probability set to never, and all other
edges not set.
I've committed two patches for expand_oacc_for that trigger the check.
OK for trunk if bootstrap and reg-test on x86_64 succeeds?
Yea, but I'd like to see ongoing work towards full checking.
I have full checking in my tree for some time. At x86-64 bootstrap there
is one remaining offender in simd_clone_adjust which was not fixed yet
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00219.html
Jakub did not tell me what would be a reasonable guess :)
After that I plan to enable full checking after checking arm/ppc.
So I hope we will converge to full checking really soon. But having
additional check is fine.
To make sure I understand correctly: are you saying that you have in a
local tree:
- patches that add missing edge probabilities, and
- a patch that adds a check that all edges have probabilities,
and that the state of that local tree is that for x86_64 bootstrap and
reg-test there only one regression site left?
If that is the case, I'd better stop fixing sites that trigger the check
in my patch.
Thanks,
- Tom
Verify edge probability consistency in verify_flow_info
2017-08-02 Tom de Vries
* cfghooks.c (verify_flow_info): Verify that if one outgoing edge has
probability set, all outgoing edges have probability set.
---
gcc/cfghooks.c | 16
1 file changed, 16 insertions(+)
diff --git a/gcc/cfghooks.c b/gcc/cfghooks.c
index 18dc49a..a9af41f 100644
--- a/gcc/cfghooks.c
+++ b/gcc/cfghooks.c
@@ -152,6 +152,8 @@ verify_flow_info (void)
bb->index, bb->frequency);
err = 1;
}
+ bool has_prob_uninit_edges = false;
+ bool has_prob_init_edges = false;
FOR_EACH_EDGE (e, ei, bb->succs)
{
if (last_visited [e->dest->index] == bb)
@@ -166,6 +168,13 @@ verify_flow_info (void)
e->src->index, e->dest->index);
err = 1;
}
+ if ((e->flags & EDGE_EH) == 0)
+ {
+ if (e->probability.initialized_p ())
+ has_prob_init_edges = true;
+ else
+ has_prob_uninit_edges = true;
+ }
if (!e->count.verify ())
{
error ("verify_flow_info: Wrong count of edge %i->%i",
@@ -197,6 +206,13 @@ verify_flow_info (void)
error ("wrong amount of branch edges after unconditional jump %i", bb->index);
err = 1;
}
+ if (has_prob_uninit_edges && has_prob_init_edges
+ && EDGE_COUNT (bb->succs) == 2)
+ {
+ error ("Missing edge probability after unconditional jump in bb %i",
+ bb->index);
+ err = 1;
+ }
FOR_EACH_EDGE (e, ei, bb->preds)
{
Re: [PATCH] Verify edge probability consistency in verify_flow_info
On 08/04/2017 11:15 AM, Jan Hubicka wrote: OK for trunk if bootstrap and reg-test on x86_64 succeeds? Yea, but I'd like to see ongoing work towards full checking. I have full checking in my tree for some time. At x86-64 bootstrap there is one remaining offender in simd_clone_adjust which was not fixed yet https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00219.html Jakub did not tell me what would be a reasonable guess :) I think I just fixed that one here ( https://gcc.gnu.org/ml/gcc-cvs/2017-08/msg00112.html ). Thanks, - Tom
Re: [PATCH] Verify edge probability consistency in verify_flow_info
> >
> > III.
> >
> > I've written this patch to check for the missing probability more
> > consistently. I'm not certain if we can require that the probability
> > should always be set, so I'm just requiring that if it is set on one
> > outgoing edge, it needs to be set on all outgoing edges.
> >
> > Sofar I've build a c-only x86_64 non-bootstrap compiler and ran dg.exp.
> > The only problem I ran into was in attr-simd{,-2,-4}.c. I've written a
> > tentative patch for that, and will submit it as follow-up.
> >
> > Is this check a good idea?
> I think the additional checking is a good idea. Ideally we'd verify
> that all edges have a probability. Until then I think you need some
> kind of rationale in a comment for why the checking is limited.
>
> >
> > OK for trunk if bootstrap and reg-test on x86_64 succeeds?
> Yea, but I'd like to see ongoing work towards full checking.
I have full checking in my tree for some time. At x86-64 bootstrap there
is one remaining offender in simd_clone_adjust which was not fixed yet
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00219.html
Jakub did not tell me what would be a reasonable guess :)
After that I plan to enable full checking after checking arm/ppc.
So I hope we will converge to full checking really soon. But having
additional check is fine.
Honza
>
> Jeff
Re: [PATCH] Verify edge probability consistency in verify_flow_info
On 08/02/2017 10:07 AM, Tom de Vries wrote:
> Hi,
>
> I.
>
> for target nvptx we recently ran into PR81442, an ICE in verify_flow_info:
> ...
> error: verify_flow_info: REG_BR_PROB is set but cfg probability is not
> ...
>
> We start out with a jump instruction:
> ...
> (jump_insn 18 17 31 2 (set (pc)
> (if_then_else (ne (reg:BI 83)
> (const_int 0 [0]))
> (label_ref 31)
> (pc))) "reduction-5.c":52 104 {br_true}
> (expr_list:REG_DEAD (reg:BI 83)
> (int_list:REG_BR_PROB 1 (nil)))
> -> 31)
> ...
>
> The probability is set on the branch edge, but not on the fallthru edge.
>
> After fixup_reorder_chain, the condition is reversed, and the
> probability reg-note update accordingly:
> ...
> (jump_insn 18 17 32 2 (set (pc)
> (if_then_else (eq (reg:BI 83)
> (const_int 0 [0]))
> (label_ref:DI 33)
> (pc))) "reduction-5.c":52 105 {br_false}
> (expr_list:REG_DEAD (reg:BI 83)
> (int_list:REG_BR_PROB 0 (nil)))
> -> 33)
> ...
>
> The branch and fallthru edge are also reversed, which means now the
> probability is set on the fallthru edge, and not on the branch edge.
>
> This is the immediate cause for the verify_flow_info error.
>
>
> II.
>
> We've fixed the ICE in the target by setting the probability on the
> fallthru edge when we introduce it (r250422).
>
> We've noted other places where we were forgetting to set the probability
> (fixed in r250823), but that did not trigger the ICE.
>
>
> III.
>
> I've written this patch to check for the missing probability more
> consistently. I'm not certain if we can require that the probability
> should always be set, so I'm just requiring that if it is set on one
> outgoing edge, it needs to be set on all outgoing edges.
>
> Sofar I've build a c-only x86_64 non-bootstrap compiler and ran dg.exp.
> The only problem I ran into was in attr-simd{,-2,-4}.c. I've written a
> tentative patch for that, and will submit it as follow-up.
>
> Is this check a good idea?
I think the additional checking is a good idea. Ideally we'd verify
that all edges have a probability. Until then I think you need some
kind of rationale in a comment for why the checking is limited.
>
> OK for trunk if bootstrap and reg-test on x86_64 succeeds?
Yea, but I'd like to see ongoing work towards full checking.
Jeff
[PATCH] Verify edge probability consistency in verify_flow_info
Hi,
I.
for target nvptx we recently ran into PR81442, an ICE in verify_flow_info:
...
error: verify_flow_info: REG_BR_PROB is set but cfg probability is not
...
We start out with a jump instruction:
...
(jump_insn 18 17 31 2 (set (pc)
(if_then_else (ne (reg:BI 83)
(const_int 0 [0]))
(label_ref 31)
(pc))) "reduction-5.c":52 104 {br_true}
(expr_list:REG_DEAD (reg:BI 83)
(int_list:REG_BR_PROB 1 (nil)))
-> 31)
...
The probability is set on the branch edge, but not on the fallthru edge.
After fixup_reorder_chain, the condition is reversed, and the
probability reg-note update accordingly:
...
(jump_insn 18 17 32 2 (set (pc)
(if_then_else (eq (reg:BI 83)
(const_int 0 [0]))
(label_ref:DI 33)
(pc))) "reduction-5.c":52 105 {br_false}
(expr_list:REG_DEAD (reg:BI 83)
(int_list:REG_BR_PROB 0 (nil)))
-> 33)
...
The branch and fallthru edge are also reversed, which means now the
probability is set on the fallthru edge, and not on the branch edge.
This is the immediate cause for the verify_flow_info error.
II.
We've fixed the ICE in the target by setting the probability on the
fallthru edge when we introduce it (r250422).
We've noted other places where we were forgetting to set the probability
(fixed in r250823), but that did not trigger the ICE.
III.
I've written this patch to check for the missing probability more
consistently. I'm not certain if we can require that the probability
should always be set, so I'm just requiring that if it is set on one
outgoing edge, it needs to be set on all outgoing edges.
Sofar I've build a c-only x86_64 non-bootstrap compiler and ran dg.exp.
The only problem I ran into was in attr-simd{,-2,-4}.c. I've written a
tentative patch for that, and will submit it as follow-up.
Is this check a good idea?
OK for trunk if bootstrap and reg-test on x86_64 succeeds?
Thanks,
- Tom
Verify edge probability consistency in verify_flow_info
2017-08-02 Tom de Vries
* cfghooks.c (verify_flow_info): Verify that if one outgoing edge has
probability set, all outgoing edges have probability set.
---
gcc/cfghooks.c | 12
1 file changed, 12 insertions(+)
diff --git a/gcc/cfghooks.c b/gcc/cfghooks.c
index 18dc49a..b973651 100644
--- a/gcc/cfghooks.c
+++ b/gcc/cfghooks.c
@@ -152,6 +152,8 @@ verify_flow_info (void)
bb->index, bb->frequency);
err = 1;
}
+ bool has_prob_uninit_edges = false;
+ bool has_prob_init_edges = false;
FOR_EACH_EDGE (e, ei, bb->succs)
{
if (last_visited [e->dest->index] == bb)
@@ -166,6 +168,10 @@ verify_flow_info (void)
e->src->index, e->dest->index);
err = 1;
}
+ if (e->probability.initialized_p ())
+ has_prob_init_edges = true;
+ else
+ has_prob_uninit_edges = true;
if (!e->count.verify ())
{
error ("verify_flow_info: Wrong count of edge %i->%i",
@@ -197,6 +203,12 @@ verify_flow_info (void)
error ("wrong amount of branch edges after unconditional jump %i", bb->index);
err = 1;
}
+ if (has_prob_uninit_edges && has_prob_init_edges)
+ {
+ error ("Missing edge probability after unconditional jump in bb %i",
+ bb->index);
+ err = 1;
+ }
FOR_EACH_EDGE (e, ei, bb->preds)
{
