Re: basic_block flags, BB_VISITED

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Oct 14, 2016 at 10:01 AM, Thomas Schwinge
<thomas@codesourcery.com> wrote:
> Hi!
>
> After the "Add Early VRP" GCC trunk commit r240291 (Kugan CC for your
> information), I've been observing all kinds of OpenACC offloading
> failures.  I now figured out what's going on.
>
> The "evrp" pass uses basic_block's BB_VISITED flag.  It first clears
> these all, gcc/tree-vrp.c:execute_early_vrp:
>
>       FOR_EACH_BB_FN (bb, cfun)
>         {
>           bb->flags &= ~BB_VISITED;
>
> ..., then does its processing, and at the end, clears these again:
>
>       FOR_EACH_BB_FN (bb, cfun)
>         bb->flags &= ~BB_VISITED;
>
> I note that this looks slightly different from what
> gcc/cfg.c:clear_bb_flags whould be doing, which works from
> ENTRY_BLOCK_PTR_FOR_FN onwards:
>
>     /* Clear all basic block flags that do not have to be preserved.  */
>     void
>     clear_bb_flags (void)
>     {
>       basic_block bb;
>
>       FOR_BB_BETWEEN (bb, ENTRY_BLOCK_PTR_FOR_FN (cfun), NULL, next_bb)
>         bb->flags &= BB_FLAGS_TO_PRESERVE;
>     }
>
> In the specific case that I've been looking at, "evrp" processing doesn't
> change the code other than for shifting some IDs because of adding a
> (dummy one-argument) PHI node.  And the problem now exactly is that the
> ENTRY_BLOCK_PTR_FOR_FN's BB_VISITED flag is still set, and so
> gcc/omp-low.c:oacc_loop_discover_walk will bail out without doing any
> processing:

The BB_VISITED flag has indetermined state at the beginning of a pass.
You have to ensure it is cleared yourself.

EVRP clearing it (similar to tree-ssa-propagate.c) is because IRA has
the same issue and crashes on "stale" BB_VISTED flags.

Richard.

>     /* DFS walk of basic blocks BB onwards, creating OpenACC loop
>        structures as we go.  By construction these loops are properly
>        nested.  */
>
>     static void
>     oacc_loop_discover_walk (oacc_loop *loop, basic_block bb)
>     {
>     [...]
>       if (bb->flags & BB_VISITED)
>         return;
>
>      follow:
>       bb->flags |= BB_VISITED;
>     [...]
>
>     /* Discover the OpenACC loops marked up by HEAD and TAIL markers for
>        the current function.  */
>
>     static oacc_loop *
>     oacc_loop_discovery ()
>     {
>       basic_block bb;
>
>       oacc_loop *top = new_oacc_loop_outer (current_function_decl);
>       oacc_loop_discover_walk (top, ENTRY_BLOCK_PTR_FOR_FN (cfun));
>     [...]
>
> Now, gcc/cfg-flags.def says:
>
>        [Most of the basic block] flags may be cleared by clear_bb_flags().  It is generally
>        a bad idea to rely on any flags being up-to-date.  */
>     [...]
>     /* A general visited flag for passes to use.  */
>     DEF_BASIC_BLOCK_FLAG(VISITED, 13)
>
> The BB_VISITED flag to me seems to always be of very local use only.
> Should we be more strict and strengthen the above "may be"/"bad idea"
> wording?
>
> Does the "evrp" pass need to get changes applied, to properly clear
> BB_VISITED (ENTRY_BLOCK_PTR_FOR_FN, in particular)?  Or, in contrast, as
> we're not to "rely on any flags being up-to-date", should the BB_VISITED
> clearing at the end of gcc/tree-vrp.c:execute_early_vrp be removed in
> fact?
>
> From (really just) a quick look, I can't tell the exact policy that other
> GCC passes are applying/using regarding the basic_block flags...
>
> The following patch does cure the testsuite regressions:
>
> --- gcc/omp-low.c
> +++ gcc/omp-low.c
> @@ -19342,6 +19342,7 @@ oacc_loop_discovery ()
>    basic_block bb;
>
>    oacc_loop *top = new_oacc_loop_outer (current_function_decl);
> +  clear_bb_flags ();
>    oacc_loop_discover_walk (top, ENTRY_BLOCK_PTR_FOR_FN (cfun));
>
>    /* The siblings were constructed in reverse order, reverse them so
>
> Is something like that what we should do?  Should clear_bb_flags be
> called here, or early in gcc/omp-low.c:execute_oacc_device_lower (like
> some other GCC passes seem to be doing)?
>
>
> Grüße
>  Thomas