[PATCH, PR66846] Mark inner loop for fixup in parloops

[Tom de Vries]

On 16/07/15 12:15, Richard Biener wrote:
> On Thu, Jul 16, 2015 at 11:39 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> On 16/07/15 10:44, Richard Biener wrote:
>>>
>>> On Wed, Jul 15, 2015 at 9:36 PM, Tom de Vries <Tom_deVries@mentor.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> I.
>>>>
>>>> In openmp expansion of loops, we do some effort to try to create matching
>>>> loops in the loop state of the child function, f.i.in
>>>> expand_omp_for_generic:
>>>> ...
>>>>         struct loop *outer_loop;
>>>>         if (seq_loop)
>>>>           outer_loop = l0_bb->loop_father;
>>>>         else
>>>>           {
>>>>             outer_loop = alloc_loop ();
>>>>             outer_loop->header = l0_bb;
>>>>             outer_loop->latch = l2_bb;
>>>>             add_loop (outer_loop, l0_bb->loop_father);
>>>>           }
>>>>
>>>>         if (!gimple_omp_for_combined_p (fd->for_stmt))
>>>>           {
>>>>             struct loop *loop = alloc_loop ();
>>>>             loop->header = l1_bb;
>>>>             /* The loop may have multiple latches.  */
>>>>             add_loop (loop, outer_loop);
>>>>           }
>>>> ...
>>>>
>>>> And if that doesn't work out, we try to mark the loop state for fixup, in
>>>> expand_omp_taskreg and expand_omp_target:
>>>> ...
>>>>         /* When the OMP expansion process cannot guarantee an up-to-date
>>>>            loop tree arrange for the child function to fixup loops.  */
>>>>         if (loops_state_satisfies_p (LOOPS_NEED_FIXUP))
>>>>           child_cfun->x_current_loops->state |= LOOPS_NEED_FIXUP;
>>>> ...
>>>>
>>>> and expand_omp_for:
>>>> ...
>>>>     else
>>>>       /* If there isn't a continue then this is a degerate case where
>>>>          the introduction of abnormal edges during lowering will prevent
>>>>          original loops from being detected.  Fix that up.  */
>>>>       loops_state_set (LOOPS_NEED_FIXUP);
>>>> ...
>>>>
>>>> However, loops are fixed up anyway, because the first pass we execute
>>>> with
>>>> the new child function is pass_fixup_cfg.
>>>>
>>>> The new child function contains a function call to
>>>> __builtin_omp_get_num_threads, which is marked with ECF_CONST, so
>>>> execute_fixup_cfg marks the function for TODO_cleanup_cfg, and
>>>> subsequently
>>>> the loops with LOOPS_NEED_FIXUP.
>>>>
>>>>
>>>> II.
>>>>
>>>> This patch adds a verification that at the end of the omp-expand
>>>> processing
>>>> of the child function, either the loop structure is ok, or marked for
>>>> fixup.
>>>>
>>>> This verfication triggered a failure in parloops. When an outer loop is
>>>> being parallelized, both the outer and inner loop are cancelled. Then
>>>> during
>>>> omp-expansion, we create a loop in the loop state for the outer loop (the
>>>> one that is transformed), but not for the inner, which causes the
>>>> verification failure:
>>>> ...
>>>> outer-1.c:11:3: error: loop with header 5 not in loop tree
>>>> ...
>>>>
>>>> [ I ran into this verification failure with an openacc kernels testcase
>>>> on
>>>> the gomp-4_0-branch, where parloops is called additionally from a
>>>> different
>>>> location, and pass_fixup_cfg is not the first pass that the child
>>>> function
>>>> is processed by. ]
>>>>
>>>> The patch contains a bit that makes sure that the loop state of the child
>>>> function is marked for fixup in parloops. The bit is non-trival since it
>>>> create a loop state and sets the fixup flag on the loop state, but
>>>> postpones
>>>> the init_loops_structure call till move_sese_region_to_fn, where it can
>>>> succeed.
>>>>
>>>>

<SNIP>

> Can we fix the root-cause of the issue instead?  That is, build a valid loop
> structure in the first place?
>

This patch manages to keep the loop structure, that is, to not cancel 
the loop tree in parloops, and guarantee a valid loop structure at the 
end of parloops.

The transformation to insert the omp_for invalidates the loop state 
properties LOOPS_HAVE_RECORDED_EXITS and LOOPS_HAVE_SIMPLE_LATCHES, so 
we drop those in parloops.

In expand_omp_for_static_nochunk, we detect the existing loop struct of 
the omp_for, and keep it.

Then by calling pass_tree_loop_init after pass_expand_omp_ssa, we get 
the loop state properties LOOPS_HAVE_RECORDED_EXITS and 
LOOPS_HAVE_SIMPLE_LATCHES back.

Tested by running:
- gcc dg.exp "parloops*.c"
- gcc autopar.exp
- target-libgomp c.exp

Currently bootstrapping and reg-testing on x86_64.

If that succeeds, OK for trunk?

Thanks,
- Tom


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Don-t-cancel-loop-tree-in-parloops.patch
Type: text/x-patch
Size: 6026 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20150720/aa9929dc/attachment.bin>