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
