This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PING^2] Fix gcc_assert in expand_omp_for_static_chunk


On 26-11-14 14:25, Tom de Vries wrote:
On 12-11-14 11:00, Tom de Vries wrote:
Jakub,

this patch fixes a gcc_assert in expand_omp_for_static_chunk.

The assert follows a loop with composite loop condition:
...
       vec<edge_var_map> *head = redirect_edge_var_map_vector (re);
       ene = single_succ_edge (entry_bb);

       psi = gsi_start_phis (fin_bb);
       for (i = 0; !gsi_end_p (psi) && head->iterate (i, &vm);
            gsi_next (&psi), ++i)
...

AFAIU, the intention of the assert is that it tries to check that both:
- all phis have been handled (gsi_end_p (psi)), and
- all elements of head have been used (head->length () == i).
In other words, that we have stopped iterating because both loop conditions are
false.

The current assert checks that *not* all phis have been handled:
...
       gcc_assert (!gsi_end_p (psi) && i == head->length ());
...

Looking back in the history, it seems we started out with the 'all phis handled'
semantics, but I suspect that that got lost due to a typo:
...
79acaae1 2007-09-07
   gcc_assert (!phi && !args);

75a70cf95 2008-07-28
   gcc_assert (!gsi_end_p (psi) && i == VEC_length (edge_var_map, head));

f1f41a6c 2012-11-18
   gcc_assert (!gsi_end_p (psi) && i == head->length ());
...

Now, if the current assert is incorrect, why didn't it trigger?

The assert is in ssa-handling code in expand_omp_for_static_chunk. Ssa-handling
code in omp-low.c is only triggered by pass_parallelize_loops, and that pass
doesn't specify a chunk size on the GIMPLE_OMP_FOR it constructs, so that will
only call expand_omp_for_static_nochunk.

I managed to trigger this assert in my oacc kernels directive patch set (on top
of the gomp-4_0-branch), which constructs an oacc for loop in
pass_parallelize_loops, and then this code in gomp-4_0-branch has the effect
that we trigger expand_omp_for_static_chunk:
...
//TODO
   /* For OpenACC loops, force a chunk size of one, as this avoids the default
      scheduling where several subsequent iterations are being executed by the
      same thread.  */
   if (gimple_omp_for_kind (for_stmt) == GF_OMP_FOR_KIND_OACC_LOOP)
     {
       gcc_assert (fd->chunk_size == NULL_TREE);
       fd->chunk_size = build_int_cst (TREE_TYPE (fd->loop.v), 1);
     }
...

So, AFAIU, this assert (and associated ssa-handling code in
expand_omp_for_static_chunk) is dead on trunk, but I'm excercising the code
currently in my patch series, so I'd prefer to fix it rather than remove it.

Bootstrapped and reg-tested on x86_64, on top of trunk, gomp-4_0-branch and
internal oacc dev branch.

OK for trunk?


Ping^2.

Thanks,
- Tom

0001-Fix-gcc_assert-in-expand_omp_for_static_chunk.patch


2014-11-12  Tom de Vries<tom@codesourcery.com>

    * omp-low.c (expand_omp_for_static_chunk): Fix assert.
---
  gcc/omp-low.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index b59d069..5210de1 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -6775,7 +6775,7 @@ expand_omp_for_static_chunk (struct omp_region *region,
        locus = redirect_edge_var_map_location (vm);
        add_phi_arg (nphi, redirect_edge_var_map_def (vm), re, locus);
      }
-      gcc_assert (!gsi_end_p (psi) && i == head->length ());
+      gcc_assert (gsi_end_p (psi) && i == head->length ());
        redirect_edge_var_map_clear (re);
        while (1)
      {
-- 1.9.1




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]