diff mbox

Verify edge probability consistency in verify_flow_info

Message ID bea84e93-f6c7-ebfd-9fcc-3463890a116f@mentor.com
State New
Headers show

Commit Message

Tom de Vries Aug. 6, 2017, 10:24 a.m. UTC 
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
diff mbox

Patch

Verify edge probability consistency in verify_flow_info

2017-08-02  Tom de Vries  <tom@codesourcery.com>

	* 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)
 	{