[PATCH] [PR105455] predict: Check for no REG_BR_PROB in uninitialized case

[PATCH] [PR105455] predict: Check for no REG_BR_PROB in uninitialized case

[Alexandre Oliva]

There is an assumption in force_edge_cold that, if any edge out of the
same src block has uninitialized probability, then a conditional
branch out of src won't have REG_BR_PROB set.

This assumption is supposed to hold, but buggy gimple passes may turn
unconditional edges into conditional ones, adding edges with
uninitialized probability out of blocks that retain originally
unconditional edges with precise always probability.  Expand may then
copy the formerly-unconditional edge's probability to REG_BR_PROB, and
if that edge ends up forced cold, the probability in the edge will be
modified without adjusting the note, and rtl_verify_edges complains
about that.

This patch adds checking that REG_BR_PROB is absent to the path taken
by force_cold_edge for uninitialized probabilities, so that the
problem is caught earlier and fixed sooner.

I'm not sure it buys us much, but...  Regstrapped on x86_64-linux-gnu.
Ok to install?


for  gcc/ChangeLog

	* predict.cc (force_edge_cold): Check for no REG_BR_PROB in
	the uninitialized probability case.
---
 gcc/predict.cc |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/gcc/predict.cc b/gcc/predict.cc
index 5734e4c851630..48ac81624ec4a 100644
--- a/gcc/predict.cc
+++ b/gcc/predict.cc
@@ -4378,7 +4378,19 @@ force_edge_cold (edge e, bool impossible)
   /* If we are not guessing profiles but have some other edges out,
      just assume the control flow goes elsewhere.  */
   if (uninitialized_exit)
-    e->probability = goal;
+    {
+      e->probability = goal;
+#if CHECKING_P
+      /* We don't expect to have a REG_BR_PROB note to adjust when
+	 there were edges with uninitialized probabilities.  This
+	 would be a symptom of creating edges, before expand, without
+	 assigning probabilities, while other edges have them. */
+      if (current_ir_type () != IR_GIMPLE
+	  && e->src != ENTRY_BLOCK_PTR_FOR_FN (cfun))
+	gcc_checking_assert (!find_reg_note (BB_END (e->src),
+					     REG_BR_PROB, NULL_RTX));
+#endif
+    }
   /* If there are other edges out of e->src, redistribute probabilitity
      there.  */
   else if (prob_sum > profile_probability::never ())

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [PATCH] [PR105455] predict: Check for no REG_BR_PROB in uninitialized case

[Jeff Law]

On 5/11/2022 7:48 PM, Alexandre Oliva via Gcc-patches wrote:
> There is an assumption in force_edge_cold that, if any edge out of the
> same src block has uninitialized probability, then a conditional
> branch out of src won't have REG_BR_PROB set.
>
> This assumption is supposed to hold, but buggy gimple passes may turn
> unconditional edges into conditional ones, adding edges with
> uninitialized probability out of blocks that retain originally
> unconditional edges with precise always probability.  Expand may then
> copy the formerly-unconditional edge's probability to REG_BR_PROB, and
> if that edge ends up forced cold, the probability in the edge will be
> modified without adjusting the note, and rtl_verify_edges complains
> about that.
>
> This patch adds checking that REG_BR_PROB is absent to the path taken
> by force_cold_edge for uninitialized probabilities, so that the
> problem is caught earlier and fixed sooner.
>
> I'm not sure it buys us much, but...  Regstrapped on x86_64-linux-gnu.
> Ok to install?
>
>
> for  gcc/ChangeLog
>
> 	* predict.cc (force_edge_cold): Check for no REG_BR_PROB in
> 	the uninitialized probability case.
Should that be a runtime test (flag_checking) rather than a 
compile/configure time test (#if CHECKING_P)?  I think we generally 
perfer the former these days.   If you strongly think it should be a #if 
CHECKING_P, that's fine.  I just want you to ponder if the runtime test 
is more appropriate or not and change if you think it's warranted.

OK either way.

Jeff