On Fri, Oct 14, 2016 at 10:01 AM, Thomas Schwinge
<tho...@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
