On Fri, Mar 18, 2022 at 03:42:48PM +0100, Tom de Vries wrote:
> > And for NVPTX we somehow lower the taskloop into GIMPLE_ASM
> > or how we end up ICEing?
> >
>
> In the nvptx backend, gen_comment (triggering not very frequently atm) uses
> gen_rtx_ASM_INPUT_loc with as location argument DECL_SOURCE_LOCATION
> (cfun->decl).
Ok.
> Alternatively, if there's a better way to get some random valid location
> than DECL_SOURCE_LOCATION (cfun->decl), that would also work for me. ]
>
> > No objection against doing that, but if we do it, we should probably do it
> > for all or at least most gimple_build_omp_* calls, not just these 2.
> > So in gimplify_omp_parallel, gimplify_omp_task, another spot in
> > gimplify_omp_for beyond these 2, gimplify_omp_workshare (ideally just
> > in one spot for all the cases), gimplify_omp_target_update,
> > gimplify_omp_atomic, gimplify_omp_ordered, gimplify_expr's
> > case OMP_* that call gimple_build_omp_*.
> > Or is it normally handled using
> > if (!gimple_seq_empty_p (internal_post))
> > {
> > annotate_all_with_location (internal_post, input_location);
> > gimplify_seq_add_seq (pre_p, internal_post);
> > }
> > and we just need to catch the cases where we gimplify something into
> > multiple nested stmts because annotate_all_with_location doesn't
> > walk into gimple_omp_body?
>
> I can try to update the patch to take care of these additional cases.
>
> I reckon answering the questions that you're asking requires writing
> test-cases for all of these.
Actually, in the light of annotate_all_with_location annotating
the newly generated sequence except for the stmts in nested contexts
I think only the two spots you have in your patch is what needs adjusting.
But I'd do it only when actually dealing with a OMP_TASKLOOP, so both
in the spot of your second hunk and for consistency with the
annotate_all_with_location do there (pseudo patch):
+ gimple_set_location (gfor, input_location);
g = gimple_build_bind (NULL_TREE, gfor, NULL_TREE);
g = gimple_build_omp_task (g, task_clauses, NULL_TREE, NULL_TREE,
NULL_TREE, NULL_TREE, NULL_TREE);
gimple_omp_task_set_taskloop_p (g, true);
+ gimple_set_location (g, input_location);
g = gimple_build_bind (NULL_TREE, g, NULL_TREE);
gomp_for *gforo
= gimple_build_omp_for (g, GF_OMP_FOR_KIND_TASKLOOP, outer_for_clauses,
gimple_omp_for_collapse (gfor),
gimple_omp_for_pre_body (gfor));
gimple_omp_for_set_pre_body (gfor, NULL);
gimple_omp_for_set_combined_p (gforo, true);
gimple_omp_for_set_combined_into_p (gfor, true);
In theory we could do it for the gimple_build_bind results too, but we don't
do that in other spots where we gimple_build_bind in OpenMP/OpenACC related
gimplification.
Ok for trunk with those tweaks.
Jakub
