basic_block flags, BB_VISITED

[Thomas Schwinge]

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:

    /* 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