From patchwork Fri Jul 26 18:34:18 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Tobias Burnus X-Patchwork-Id: 1965389 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=baylibre-com.20230601.gappssmtp.com header.i=@baylibre-com.20230601.gappssmtp.com header.a=rsa-sha256 header.s=20230601 header.b=Sbo8tMff; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4WVxJ12t1kz1ybY for ; Sat, 27 Jul 2024 04:34:57 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 6EC1D385C6C1 for ; Fri, 26 Jul 2024 18:34:55 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com [IPv6:2a00:1450:4864:20::22e]) by sourceware.org (Postfix) with ESMTPS id AB8023858D26 for ; Fri, 26 Jul 2024 18:34:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AB8023858D26 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=baylibre.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org AB8023858D26 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::22e ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1722018866; cv=none; b=ZI7J9X0HJAHRdFgFMLwcvEcjZZZjfEcnTtWc7Y7OkMOSRYnkg6UXG18fp1LRv0dobVZL2boPYoSQ8oWUVWsUoy6tNasy6Ez2D+AYRqOLEOU9vwEDtHJc4MEOeXx62gAOA4hWpHgbwekn4UqbQDOitILNZsBoBv4LvObVm2dbSTY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1722018866; c=relaxed/simple; bh=d7x584P2YHMUObk+MKRxsfmA/Wh2XLRwV4wAXa8+FwE=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:From:To; b=Saiel6K383AsZGAJ0bCgWcQjWcWUPoGFi+ulweD9w/R2geT7uELGfccya+csVLq5wYd9OAZnVsokgxPcJQwXxR5R9mxxAMKCxx+DUaMz3L3ExLanKRlrWMMyvF5id1Qhn2SnaAmpctb2hLjd1HETufdapCku7f4vm2ztuSMv2o0= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lj1-x22e.google.com with SMTP id 38308e7fff4ca-2ef283c58f4so17632651fa.1 for ; Fri, 26 Jul 2024 11:34:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20230601.gappssmtp.com; s=20230601; t=1722018861; x=1722623661; darn=gcc.gnu.org; h=in-reply-to:content-language:references:to:from:subject:user-agent :mime-version:date:message-id:from:to:cc:subject:date:message-id :reply-to; bh=BKkX2sZaXlNli60IP5Vw+4pZRJqgLFj2MeFvjeb1oAk=; b=Sbo8tMff/eNusUNexFZLOMj4JpemWYMhojB/jjUEMgB9DGqO8Xd6+sIFjrHUlAztHD Ridv0ufWa1KL2eyJUJgqjVjdca5TuMPub7YpHbe3r1KDroca643W5CF9Z6ZHXE9rhpai JOvvlNdfLEtBPSyvhHCd3ozhlhicicVNvMpWXNWHlQ/yyqfmxdTKtj1PuGjy51RLKVjI QnWkJKd4NM0pEtcpXrJfCDscgAhFsFj/ZRJIma+FM7Jer89lhsYHX6XVjkdb8Me/9ElO dAmefrEWoR2SFNrPq5hVNBei2XIPSic/J25bkcR3xHGo9aDSdTmDbJFVkRC/pqhxV6+Y hTlA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722018861; x=1722623661; h=in-reply-to:content-language:references:to:from:subject:user-agent :mime-version:date:message-id:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=BKkX2sZaXlNli60IP5Vw+4pZRJqgLFj2MeFvjeb1oAk=; b=FcXROKRoyhqgDHXLrzGAt1tysJymZetVmd9Wj+OIZ83EHgzojh4FMsipdKvxG0A6aS L9CzjasO1MwY527CoM74vRvEpzOZjjeh0dBMim7BkhCrb6ct6YnZzaYDaeKUgVbawrCk GqSpOX706nEPE8e3yAIo3rePkm5cg2Os9eRLJBWnIKDf161uSiqG8JqKNHqsI18K9XR2 jBy90uso+EsRZUk9i2Ik8BeNGHTzXQtv9CG3uMul74iisvYfttzYBjSq6nGmViCV/a9u RmdN+6IKmqey786lM4wpH9+mXmGlz0c4G5wwQnMXmSbeXth30jJV4VBZs0ELlweXq2NE 3/tQ== X-Gm-Message-State: AOJu0YzLCil4/Dig9Wbwih9FqhRZbT7wuIvrqMSmSyNoaHZ7YTZ9S69W JjF6+QDj0hN+uIrUufFDs67FkZTe/z3AbTra4jmaeCJ9AepHg4RjMSw8Jx7JSJ31w0hJW/EECfD awXA= X-Google-Smtp-Source: AGHT+IGm7ynr43yjWd9pJ7Ir58z97H3xGwRIvQg3t4z+F8oV2TqbarTNEZ471yNhVfUO6uT58C7nBw== X-Received: by 2002:a2e:7d02:0:b0:2ef:2247:987b with SMTP id 38308e7fff4ca-2f12ee5bc33mr3275821fa.32.1722018860800; Fri, 26 Jul 2024 11:34:20 -0700 (PDT) Received: from [192.168.8.100] (tmo-085-80.customers.d1-online.com. [80.187.85.80]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4280f484cdesm23492175e9.44.2024.07.26.11.34.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 26 Jul 2024 11:34:20 -0700 (PDT) Message-ID: <2cf80995-6ba1-4aca-95af-904dc8d849bf@baylibre.com> Date: Fri, 26 Jul 2024 20:34:18 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: [Patch, v2] OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555] From: Tobias Burnus To: gcc-patches , Jakub Jelinek , "fortran@gcc.gnu.org" References: <2eb94e65-e7a9-4dcb-9ea4-963e8ffa86a9@baylibre.com> Content-Language: en-US In-Reply-To: <2eb94e65-e7a9-4dcb-9ea4-963e8ffa86a9@baylibre.com> X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, HTML_MESSAGE, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org Updated patch - only change is to the testcase: * With the just posted patch for PR116107, array sections with offset work for 'link', hence, I updated the testcase. * For 'arr2', I added ref to the associated PR. I intent to commit it once PR116107 has been committed. Tobias Tobias Burnus wrote: > Hi all, > > it turned out that 'declare target' with 'link' clause was broken in multiple ways. > > The main fix is the attached patch, i.e. namely pushing the variables already to > the offload-vars list already in the FE. > > When implementing it, I noticed: > * C has a similar issue when using nested functions, which is > a GNU extension →https://gcc.gnu.org/115574 > > * When doing partial mapping of arrays (which is one of the reasons for 'link'), > offsets are mishandled in Fortran (not tested in C), see FIXME in the patch) > There: arr2(10) should print 10 but with map(arr2(10:)) it prints 19. > (I will file a PR about this). > > * It might happen that linked variables do not get linked. I have not investigated > why, but 'arr2' gives link errors – while 'arr' works. > See FIXME in the patch. (I will file a PR about this) > > * For COMMON blocks, map(/common/) is rejected,https://gcc.gnu.org/PR115577 > > * When then mapping map(a,b,c) which is identical for 'common /mycom/ a,b,c', > it fails to link the device side as the 'mycom_' symbol cannot be found on the > device side. (I will file a PR about this) > > As COMMON as issues, an alternative would be to defer the trans-common.cc > changes to a later patch. > > Comments, questions, concerns? > > Tobias > > PS: Tested with nvptx offloading with a page-migration supporting system with > nvptx and GCN offloading configured and no new fails observed. OpenMP/Fortran: Fix handling of 'declare target' with 'link' clause [PR11555] Contrary to a normal 'declare target', the 'declare target link' attribute also needs to set node->offloadable and push the offload_vars in the front end. Linked variables require that the data is mapped. For module variables, this can happen anywhere. For variables in an external subprograms or the main programm, this can only happen in the either that program itself or in an internal subprogram. - Whether a variable is just normally mapped or linked then becomes relevant if a device routine exists that can access that variable, i.e. an internal procedure has then to be marked as declare target. PR fortran/115559 gcc/fortran/ChangeLog: * trans-common.cc (build_common_decl): Add 'omp declare target' and 'omp declare target link' variables to offload_vars. * trans-decl.cc (add_attributes_to_decl): Likewise; update args and call decl_attributes. (get_proc_pointer_decl, gfc_get_extern_function_decl, build_function_decl): Update calls. (gfc_get_symbol_decl): Likewise; move after 'DECL_STATIC (t)=1' to avoid errors with symtab_node::get_create. libgomp/ChangeLog: * testsuite/libgomp.fortran/declare-target-link.f90: New test. gcc/fortran/trans-common.cc | 21 ++++ gcc/fortran/trans-decl.cc | 81 +++++++++----- .../libgomp.fortran/declare-target-link.f90 | 116 +++++++++++++++++++++ 3 files changed, 192 insertions(+), 26 deletions(-) diff --git a/gcc/fortran/trans-common.cc b/gcc/fortran/trans-common.cc index 5f44e7bd663..e714342c3c0 100644 --- a/gcc/fortran/trans-common.cc +++ b/gcc/fortran/trans-common.cc @@ -98,6 +98,9 @@ along with GCC; see the file COPYING3. If not see #include "coretypes.h" #include "tm.h" #include "tree.h" +#include "cgraph.h" +#include "context.h" +#include "omp-offload.h" #include "gfortran.h" #include "trans.h" #include "stringpool.h" @@ -497,6 +500,24 @@ build_common_decl (gfc_common_head *com, tree union_type, bool is_init) = tree_cons (get_identifier ("omp declare target"), omp_clauses, DECL_ATTRIBUTES (decl)); + if (com->omp_declare_target_link || com->omp_declare_target) + { + /* Add to offload_vars; get_create does so for omp_declare_target, + omp_declare_target_link requires manual work. */ + gcc_assert (symtab_node::get (decl) == 0); + symtab_node *node = symtab_node::get_create (decl); + if (node != NULL && com->omp_declare_target_link) + { + node->offloadable = 1; + if (ENABLE_OFFLOADING) + { + g->have_offload = true; + if (is_a (node)) + vec_safe_push (offload_vars, decl); + } + } + } + /* Place the back end declaration for this common block in GLOBAL_BINDING_LEVEL. */ gfc_map_of_all_commons[identifier] = pushdecl_top_level (decl); diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc index 82fa2bb6134..0fdc41b1784 100644 --- a/gcc/fortran/trans-decl.cc +++ b/gcc/fortran/trans-decl.cc @@ -46,7 +46,9 @@ along with GCC; see the file COPYING3. If not see #include "trans-stmt.h" #include "gomp-constants.h" #include "gimplify.h" +#include "context.h" #include "omp-general.h" +#include "omp-offload.h" #include "attr-fnspec.h" #include "tree-iterator.h" #include "dependency.h" @@ -1472,19 +1474,18 @@ gfc_add_assign_aux_vars (gfc_symbol * sym) } -static tree -add_attributes_to_decl (symbol_attribute sym_attr, tree list) +static void +add_attributes_to_decl (tree *decl_p, const gfc_symbol *sym) { unsigned id; - tree attr; + tree list = NULL_TREE; + symbol_attribute sym_attr = sym->attr; for (id = 0; id < EXT_ATTR_NUM; id++) if (sym_attr.ext_attr & (1 << id) && ext_attr_list[id].middle_end_name) { - attr = build_tree_list ( - get_identifier (ext_attr_list[id].middle_end_name), - NULL_TREE); - list = chainon (list, attr); + tree ident = get_identifier (ext_attr_list[id].middle_end_name); + list = tree_cons (ident, NULL_TREE, list); } tree clauses = NULL_TREE; @@ -1547,6 +1548,7 @@ add_attributes_to_decl (symbol_attribute sym_attr, tree list) clauses = c; } + bool has_declare = true; if (sym_attr.omp_declare_target_link || sym_attr.oacc_declare_link) list = tree_cons (get_identifier ("omp declare target link"), @@ -1558,12 +1560,45 @@ add_attributes_to_decl (symbol_attribute sym_attr, tree list) || sym_attr.oacc_declare_device_resident) list = tree_cons (get_identifier ("omp declare target"), clauses, list); + else + has_declare = false; if (sym_attr.omp_declare_target_indirect) list = tree_cons (get_identifier ("omp declare target indirect"), clauses, list); - return list; + decl_attributes (decl_p, list, 0); + + if (has_declare + && VAR_P (*decl_p) + && sym->ns->proc_name->attr.flavor != FL_MODULE) + { + has_declare = false; + for (gfc_namespace* ns = sym->ns->contained; ns; ns = ns->sibling) + if (ns->proc_name->attr.omp_declare_target) + { + has_declare = true; + break; + } + } + + if (has_declare && VAR_P (*decl_p) && has_declare) + { + /* Add to offload_vars; get_create does so for omp_declare_target, + omp_declare_target_link requires manual work. */ + gcc_assert (symtab_node::get (*decl_p) == 0); + symtab_node *node = symtab_node::get_create (*decl_p); + if (node != NULL && sym_attr.omp_declare_target_link) + { + node->offloadable = 1; + if (ENABLE_OFFLOADING) + { + g->have_offload = true; + if (is_a (node)) + vec_safe_push (offload_vars, *decl_p); + } + } + } } @@ -1578,7 +1613,6 @@ gfc_get_symbol_decl (gfc_symbol * sym) { tree decl; tree length = NULL_TREE; - tree attributes; int byref; bool intrinsic_array_parameter = false; bool fun_or_res; @@ -1864,12 +1898,6 @@ gfc_get_symbol_decl (gfc_symbol * sym) decl = build_decl (gfc_get_location (&sym->declared_at), VAR_DECL, gfc_sym_identifier (sym), gfc_sym_type (sym)); - /* Add attributes to variables. Functions are handled elsewhere. */ - attributes = add_attributes_to_decl (sym->attr, NULL_TREE); - decl_attributes (&decl, attributes, 0); - if (sym->ts.deferred && VAR_P (length)) - decl_attributes (&length, attributes, 0); - /* Symbols from modules should have their assembler names mangled. This is done here rather than in gfc_finish_var_decl because it is different for string length variables. */ @@ -2035,6 +2063,12 @@ gfc_get_symbol_decl (gfc_symbol * sym) TREE_READONLY (decl) = 1; } + /* Add attributes to variables. Functions are handled elsewhere. */ + add_attributes_to_decl (&decl, sym); + + if (sym->ts.deferred && VAR_P (length)) + decl_attributes (&length, DECL_ATTRIBUTES (decl), 0); + return decl; } @@ -2071,7 +2105,6 @@ static tree get_proc_pointer_decl (gfc_symbol *sym) { tree decl; - tree attributes; if (sym->module || sym->fn_result_spec) { @@ -2151,8 +2184,7 @@ get_proc_pointer_decl (gfc_symbol *sym) && (TREE_STATIC (decl) || DECL_EXTERNAL (decl))) set_decl_tls_model (decl, decl_default_tls_model (decl)); - attributes = add_attributes_to_decl (sym->attr, NULL_TREE); - decl_attributes (&decl, attributes, 0); + add_attributes_to_decl (&decl, sym); return decl; } @@ -2166,7 +2198,6 @@ gfc_get_extern_function_decl (gfc_symbol * sym, gfc_actual_arglist *actual_args, { tree type; tree fndecl; - tree attributes; gfc_expr e; gfc_intrinsic_sym *isym; gfc_expr argexpr; @@ -2364,8 +2395,7 @@ module_sym: DECL_EXTERNAL (fndecl) = 1; TREE_PUBLIC (fndecl) = 1; - attributes = add_attributes_to_decl (sym->attr, NULL_TREE); - decl_attributes (&fndecl, attributes, 0); + add_attributes_to_decl (&fndecl, sym); gfc_set_decl_assembler_name (fndecl, mangled_name); @@ -2424,7 +2454,7 @@ module_sym: static void build_function_decl (gfc_symbol * sym, bool global) { - tree fndecl, type, attributes; + tree fndecl, type; symbol_attribute attr; tree result_decl; gfc_formal_arglist *f; @@ -2475,15 +2505,14 @@ build_function_decl (gfc_symbol * sym, bool global) if (sym->attr.referenced || sym->attr.entry_master) TREE_USED (fndecl) = 1; - attributes = add_attributes_to_decl (attr, NULL_TREE); - decl_attributes (&fndecl, attributes, 0); + add_attributes_to_decl (&fndecl, sym); /* Figure out the return type of the declared function, and build a RESULT_DECL for it. If this is a subroutine with alternate returns, build a RESULT_DECL for it. */ result_decl = NULL_TREE; /* TODO: Shouldn't this just be TREE_TYPE (TREE_TYPE (fndecl)). */ - if (attr.function) + if (sym->attr.function) { if (gfc_return_by_reference (sym)) type = void_type_node; @@ -2530,7 +2559,7 @@ build_function_decl (gfc_symbol * sym, bool global) /* Set attributes for PURE functions. A call to a PURE function in the Fortran 95 sense is both pure and without side effects in the C sense. */ - if (attr.pure || attr.implicit_pure) + if (sym->attr.pure || sym->attr.implicit_pure) { /* TODO: check if a pure SUBROUTINE has no INTENT(OUT) arguments including an alternate return. In that case it can also be diff --git a/libgomp/testsuite/libgomp.fortran/declare-target-link.f90 b/libgomp/testsuite/libgomp.fortran/declare-target-link.f90 new file mode 100644 index 00000000000..2ce212d114f --- /dev/null +++ b/libgomp/testsuite/libgomp.fortran/declare-target-link.f90 @@ -0,0 +1,116 @@ +! { dg-additional-options "-Wall" } +! PR fortran/115559 + +module m + integer :: A + !$omp declare target link(A) +end module m + +subroutine f + implicit none (type, external) + integer, save :: x, y ! { dg-warning "Unused variable 'y' declared" } + !$omp declare target link(x, y) + + ! note: y is not 'link' as gfortran doesn't regard it as used + x = 6 + call ii + +contains + subroutine k + !$omp declare target + use m + A = 5 + end + subroutine ii + integer :: res + !$omp target map(x) map(from: res) + call k() + call ll() + res = get() + !$omp end target + ! print *, res + if (res /= 6 + 7 + 5) & + stop 1 + end + subroutine ll + !$omp declare target + x = x + 7 + end + integer function get() + use m + !$omp declare target + get = x + A + end +end + + +subroutine sub + implicit none (type, external) + integer, save :: arr(100), arr2(1:4) + !$omp declare target link(arr,arr2) + + call mapit + call device1 + call re_mapit + call device2 +contains + subroutine mapit + integer :: i + arr = [(i, i=1,100)] + !$omp target enter data map(to:arr(10:50)) map(alloc: arr2(1:4)) + end subroutine + subroutine re_mapit + integer :: i + !$omp target exit data map(from:arr(10:50)) map(delete: arr2) + + if (any (arr(1:9) /= [(i, i=1,9)])) stop 2 + if (any (arr(10:50) /= [(3-10*i, i=10,50)])) stop 3 + if (any (arr(51:100) /= [(i, i=51,100)])) stop 4 + end subroutine + + subroutine device1 + integer :: res + !$omp target map(from:res) + res = run_device1() + !$omp end target + print *, res + ! FIXME: arr2 not link mapped -> PR115637 + ! if (res /= -11436) stop 5 + if (res /= -11546) stop 5 ! FIXME + end + integer function run_device1() + !$omp declare target + integer :: i + run_device1 = -99 + ! FIXME: arr2 not link mapped -> PR115637 + ! arr2 = [11,22,33,44] + if (any (arr(10:50) /= [(i, i=10,50)])) then + run_device1 = arr(11) + return + end if + ! FIXME: -> PR115637 + ! run_device1 = sum(arr(10:13) + arr2) + run_device1 = sum(arr(10:13) ) ! FIXME + do i = 10, 50 + arr(i) = 3 - 10 * arr(i) + end do + run_device1 = run_device1 + sum(arr(15:50)) + end + subroutine device2 + end + integer function run_device2() + !$omp declare target + run_device2 = -99 + end +end + + +use m +implicit none (type, external) +external f +external sub + +!$omp target enter data map(alloc: A) +call f() +call sub +end