Re: [PATCH][openmp] Set location for taskloop stmts
On 3/18/22 15:56, Jakub Jelinek wrote:
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.
Ack, committed (in two steps though, I accidentally first committed the
old patch).
Thanks,
- Tom
Re: [PATCH][openmp] Set location for taskloop stmts
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
Re: [PATCH][openmp] Set location for taskloop stmts
On 3/18/22 14:01, Jakub Jelinek wrote:
On Fri, Mar 18, 2022 at 01:44:00PM +0100, Tom de Vries wrote:
The test-case included in this patch contains:
...
#pragma omp taskloop simd shared(a) lastprivate(myId)
...
This is translated to 3 taskloop statements in gimple, visible with
-fdump-tree-gimple:
...
#pragma omp taskloop private(D.2124)
#pragma omp taskloop shared(a) shared(myId) private(i.0) firstprivate(a_h)
#pragma omp taskloop lastprivate(myId)
...
But when exposing the gimple statement locations using
-fdump-tree-gimple-lineno, we find that only the first one has location
information.
Fix this by adding the missing location information.
Tested gomp.exp on x86_64.
Tested libgomp testsuite on x86_64 with nvptx accelerator.
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).
If this location is UNKNOWN_LOCATION, we run into an ICE, which is fixed
by the proposed patch "[final] Handle compiler-generated asm insn" (
https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590721.html ).
As for the openmp test-case, we end up lowering at least one of those
taskloops into an outlined function, and if its location is
UNKNOWN_LOCATION and gen_comment is triggered in the body, we run into
the ICE.
[ My preferred solution is to have "[final] Handle compiler-generated
asm insn" approved and committed, but no response sofar, maybe ignored
for not being stage-4 material, I'm not sure.
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.
Thanks,
- Tom
Re: [PATCH][openmp] Set location for taskloop stmts
On Fri, Mar 18, 2022 at 01:44:00PM +0100, Tom de Vries wrote:
> The test-case included in this patch contains:
> ...
> #pragma omp taskloop simd shared(a) lastprivate(myId)
> ...
>
> This is translated to 3 taskloop statements in gimple, visible with
> -fdump-tree-gimple:
> ...
> #pragma omp taskloop private(D.2124)
> #pragma omp taskloop shared(a) shared(myId) private(i.0) firstprivate(a_h)
> #pragma omp taskloop lastprivate(myId)
> ...
>
> But when exposing the gimple statement locations using
> -fdump-tree-gimple-lineno, we find that only the first one has location
> information.
>
> Fix this by adding the missing location information.
>
> Tested gomp.exp on x86_64.
>
> Tested libgomp testsuite on x86_64 with nvptx accelerator.
And for NVPTX we somehow lower the taskloop into GIMPLE_ASM
or how we end up ICEing?
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?
Jakub
[PATCH][openmp] Set location for taskloop stmts
Hi,
The test-case included in this patch contains:
...
#pragma omp taskloop simd shared(a) lastprivate(myId)
...
This is translated to 3 taskloop statements in gimple, visible with
-fdump-tree-gimple:
...
#pragma omp taskloop private(D.2124)
#pragma omp taskloop shared(a) shared(myId) private(i.0) firstprivate(a_h)
#pragma omp taskloop lastprivate(myId)
...
But when exposing the gimple statement locations using
-fdump-tree-gimple-lineno, we find that only the first one has location
information.
Fix this by adding the missing location information.
Tested gomp.exp on x86_64.
Tested libgomp testsuite on x86_64 with nvptx accelerator.
OK for trunk?
Thanks,
- Tom
[openmp] Set location for taskloop stmts
gcc/ChangeLog:
2022-03-18 Tom de Vries
* gimplify.cc (gimplify_omp_for): Set taskloop location.
gcc/testsuite/ChangeLog:
2022-03-18 Tom de Vries
* c-c++-common/gomp/pr104968.c: New test.
---
gcc/gimplify.cc| 2 ++
gcc/testsuite/c-c++-common/gomp/pr104968.c | 14 ++
2 files changed, 16 insertions(+)
diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 139a0de6100..c46589639e4 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -13178,6 +13178,7 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
gfor = gimple_build_omp_for (for_body, kind, OMP_FOR_CLAUSES (orig_for_stmt),
TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt)),
for_pre_body);
+ gimple_set_location (gfor, EXPR_LOCATION (*expr_p));
if (orig_for_stmt != for_stmt)
gimple_omp_for_set_combined_p (gfor, true);
if (gimplify_omp_ctxp
@@ -13361,6 +13362,7 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
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_set_location (g, EXPR_LOCATION (*expr_p));
gimple_omp_task_set_taskloop_p (g, true);
g = gimple_build_bind (NULL_TREE, g, NULL_TREE);
gomp_for *gforo
diff --git a/gcc/testsuite/c-c++-common/gomp/pr104968.c
b/gcc/testsuite/c-c++-common/gomp/pr104968.c
new file mode 100644
index 000..2977db2f433
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/pr104968.c
@@ -0,0 +1,14 @@
+/* { dg-additional-options "-fdump-tree-gimple-lineno" } */
+
+int
+main (void)
+{
+ double a[10], a_h[10];
+ int myId = -1;
+#pragma omp target map(tofrom:a)
+#pragma omp taskloop simd shared(a) lastprivate(myId) /* { dg-line here } */
+for(int i = 0 ; i < 10; i++) if (a[i] != a_h[i]) { }
+}
+
+/* { dg-final { scan-tree-dump-times "#pragma omp taskloop" 3 "gimple" } } */
+/* { dg-final { scan-tree-dump-times "(?n)\\\[.*pr104968.c:[get-absolute-line
'' here]:.*\\\] #pragma omp taskloop" 3 "gimple" } } */
