Re: [PATCH][RFC] Sanity checking for -freorder-blocks-and-partition failures

[Teresa Johnson <tejohnson at google dot com>]

On Tue, Oct 30, 2012 at 9:26 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> Hello,
>
> Hot/cold partitioning is apparently a hot topic all of a sudden, which
> is a good thing of course, because it's in need of some TLC.
>
> The attached patch adds another check the RTL cfg checking
> (verify_flow_info) for the partitioning: A hot block can never be
> dominated by a cold block (because the dominated block must also be
> cold). This trips in PR55121.
>
> I haven't tested this with any profiling tests, but it's bound to
> break things. From my POV, whatever gets broken by this patch was
> already broken to begin with :-)   If you're in CC, it's because I
> hope you can help test this patch.

I will try testing your patch on top of mine with our fdo benchmarks.
For the others on the cc list, you may need to include my patch as
well for testing. Without it, -freorder-blocks-and-partition was DOA
for me. For my patch, see
http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html

Teresa

>
> Downside of this patch is that I need dominance info. If it's not
> available, I compute and free it. I'm not sure if this works if
> dominance info status is DOM_NO_FAST_QUERY, and I don't want to
> recompute in this case because IMHO a verifier should be a no-op from
> the POV of the rest of the compiler, and updating dominators would
> make this patch a not-a-no-op :-)
>
> Thoughts/comments?
>
> Ciao!
> Steven
>
>         * cfgrtl.c (rtl_verify_flow_info_1): Verify that blocks in the
>         hot partition are not dominated by blocks in the cold partition.
>
> Index: cfgrtl.c
> ===================================================================
> --- cfgrtl.c    (revision 191819)
> +++ cfgrtl.c    (working copy)
> @@ -2033,6 +2033,7 @@ rtl_verify_flow_info_1 (void)
>    rtx x;
>    int err = 0;
>    basic_block bb;
> +  bool have_partitions = false;
>
>    /* Check the general integrity of the basic blocks.  */
>    FOR_EACH_BB_REVERSE (bb)
> @@ -2145,6 +2146,8 @@ rtl_verify_flow_info_1 (void)
>             n_eh++;
>           else if (e->flags & EDGE_ABNORMAL)
>             n_abnormal++;
> +
> +         have_partitions |= is_crossing;
>         }
>
>        if (n_eh && !find_reg_note (BB_END (bb), REG_EH_REGION, NULL_RTX))
> @@ -2263,6 +2268,40 @@ rtl_verify_flow_info_1 (void)
>           }
>      }
>
> +  /* If there are partitions, do a sanity check on them: A basic block in
> +     a cold partition cannot dominate a basic block in a hot partition.  */
> +  VEC (basic_block, heap) *bbs_in_cold_partition = NULL;
> +  if (have_partitions && !err)
> +    FOR_EACH_BB (bb)
> +      if ((BB_PARTITION (bb) == BB_COLD_PARTITION))
> +       VEC_safe_push (basic_block, heap, bbs_in_cold_partition, bb);
> +  if (! VEC_empty (basic_block, bbs_in_cold_partition))
> +    {
> +      bool dom_calculated_here = !dom_info_available_p (CDI_DOMINATORS);
> +      basic_block son;
> +
> +      if (dom_calculated_here)
> +       calculate_dominance_info (CDI_DOMINATORS);
> +
> +      while (! VEC_empty (basic_block, bbs_in_cold_partition))
> +       {
> +         bb = VEC_pop (basic_block, bbs_in_cold_partition);
> +         if ((BB_PARTITION (bb) != BB_COLD_PARTITION))
> +           {
> +             error ("non-cold basic block %d dominated "
> +                    "by a block in the cold partition", bb->index);
> +             err = 1;
> +           }
> +         for (son = first_dom_son (CDI_DOMINATORS, bb);
> +              son;
> +              son = next_dom_son (CDI_DOMINATORS, son))
> +           VEC_safe_push (basic_block, heap, bbs_in_cold_partition, son);
> +       }
> +
> +      if (dom_calculated_here)
> +       free_dominance_info (CDI_DOMINATORS);
> +    }
> +
>    /* Clean up.  */
>    return err;
>  }



-- 
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413