Message ID | 20220318124358.GA28424@delia.home |
---|---|
State | Committed |
Commit | 356e2720e9030927579024c2f060d665a0b9080f |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 77B673888C6B for <patchwork@sourceware.org>; Fri, 18 Mar 2022 12:44:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 77B673888C6B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1647607472; bh=3q6s/vG/kxXyge3SUuCvvOoDhRDLiIJmpQbvlv7aqW4=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=aaCmDO2Sz2Riv2DkL/BAZ/vBxyCrHUQfQsMOIqL980cA5HinJ2H9ydLXF7MsG1wTz rU82dS+5h37Hj10SaXqQo2ctMeI0uz0zOJp/sodhCI7bxlJZsJYow/eKJot0w+drQm fvu1kaHFzJ6FP2Vk9ttfBNMk5kk3eDnF2Ebbnys8= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id B537C388883E for <gcc-patches@gcc.gnu.org>; Fri, 18 Mar 2022 12:44:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B537C388883E Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 9ACBC210EC; Fri, 18 Mar 2022 12:44:01 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 7EC9012FC5; Fri, 18 Mar 2022 12:44:01 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id Vh7jHZF+NGI5GQAAMHmgww (envelope-from <tdevries@suse.de>); Fri, 18 Mar 2022 12:44:01 +0000 Date: Fri, 18 Mar 2022 13:44:00 +0100 To: gcc-patches@gcc.gnu.org Subject: [PATCH][openmp] Set location for taskloop stmts Message-ID: <20220318124358.GA28424@delia.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Tom de Vries via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Tom de Vries <tdevries@suse.de> Cc: Jakub Jelinek <jakub@redhat.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[openmp] Set location for taskloop stmts
|
|
Commit Message
Tom de Vries
March 18, 2022, 12:44 p.m. UTC
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 <tdevries@suse.de> * gimplify.cc (gimplify_omp_for): Set taskloop location. gcc/testsuite/ChangeLog: 2022-03-18 Tom de Vries <tdevries@suse.de> * 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(+)
Comments
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
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
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
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
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 00000000000..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" } } */