From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 76314 invoked by alias); 29 Nov 2019 12:03:24 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 76296 invoked by uid 89); 29 Nov 2019 12:03:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=fear, 92703, sk:UNKNOWN, tons X-HELO: esa1.mentor.iphmx.com Received: from esa1.mentor.iphmx.com (HELO esa1.mentor.iphmx.com) (68.232.129.153) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 29 Nov 2019 12:03:21 +0000 IronPort-SDR: s2ZjPsS4+9XkKUOUUxaeWe2mEjv/aP9F/ZHF/pEaB3qlSYcSqdwE6bGyd1gYvq9mdiMbAgLpYT 6B8MtJ4/aIJzNZfcHyiS9UNB1h0chypBTkZ9Fn8y+gWZzPXiWuR9O+aKlQ4ZM0ZPf26xdkoCyD 8GBPeKBk8zfDeyXM7IDjOYnDwntZIJPcIRY0UOTWvOXihSnySvKmM5VGqpNcIeHWoE/1HWrZXX V92CrKR3ltomlAFHjkJFl95EUyTbuZmxpFywuP8m8Is5drFqeS9YoF97jSJlO6Ijr5vxwv58mc afw= Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa1.mentor.iphmx.com with ESMTP; 29 Nov 2019 04:03:20 -0800 IronPort-SDR: PauyFTYyyYWSHH8gDw9bgWo0V0M9mLqG22vyfPNFY3Gj07Yz0t0oG1nj0MlUZH8pkvStoIc+Ve pmnMmwkDUS2zbVUqlq3obGOXU559dKZ6oNNO1/Vjl1Pd/A3W2RmRyRh73kJNlRSIBtDTi+ODJ8 INQhJB9oXU/POkc71tZ3RayOfsiVN3doTvEonUW+mIqEjljDe+od//cXcc7UhHKY0HJQVd5kKe Kej0oW5R1XtxpiGJ6EOnRgGVpbKPEkE01qjVRYseapWD5Q78WazY2gujDlnxXhoIm9RG1h6xFT vFg= Subject: =?UTF-8?B?UmU6IFtQYXRjaF1bT3Blbk1QXSBGaXggdXNlX2RldmljZV/igKYgd2l0?= =?UTF-8?Q?h_absent_optional_arg?= From: Tobias Burnus To: gcc-patches , fortran , Jakub Jelinek References: <749d496b-3807-6438-2ed9-c44efe455bd9@mentor.com> Message-ID: Date: Fri, 29 Nov 2019 12:07:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <749d496b-3807-6438-2ed9-c44efe455bd9@mentor.com> Content-Type: multipart/mixed; boundary="------------353CEAB19A42BF964D7B979E" Return-Path: tobias@codesourcery.com X-IsSubscribed: yes X-SW-Source: 2019-11/txt/msg02635.txt.bz2 --------------353CEAB19A42BF964D7B979E Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-length: 2174 Revised patch after some re-considerations (and finding tons of issues with VALUE + OPTIONAL in gfortran itself). – What the patch does [all related to use_device_{addr,ptr} and 'optional' arguments]: For assumed-shape arrays, the compiler generates code like "if (arg) arg.0 = arg->data;". Hence, arg.0 was always available – but possibly pointing to uninitialized memory. – Likewise, per-value-passed arguments, 'arg' (i.e. &arg) is always available. — But, in the absent case, if 'arg->data is not NULL or the per-value decl is not mapped (cf. test case), that's not the best idea. I thought that I don't need a condition in thoses case – but it turns out that the offloading plugin might (rightly!) complain that the address is not mapped. – Hence, I add the is-present condition now also in for those case; as none remain, the do_optional_check is now gone. However, after the library call, amp_arr.arg is known to be initialized. In this case, I keep the do_optional_check check. – This avoids code which boils down to  "x0 = arg ? omp_arr.arg : NULL". – and keeps generating condtions only for complex code such as:  if (present) { tmp.data = omp_arr.arg; arg = &tmp; } else {arg = NULL;}. Finally, while testing/exploring value+optional bugs, I stumbled over 'type(c_ptr),value' which is 'void *'. In particular, it is pointer but still the is-present status is passed as hidden argument. This patch fixes both mapping and the is-present check. Build on x86-64-gnu-linux + tested once on a system without offloading support and with nvptx offloading. OK? Tobias PS: Regarding the issues with OPTIONAL and VALUE, see PR fortran/92703 and PR fortran/92702. Found issues: const-len character strings are passed as value w/o hidden is-present arg but func call passes them; those and derived types/the outer class container are passed by value - but the 'present()' check assumes pointers (hence: ICE); if basent null_ptr_node is passed, I fear that this will give stack issues, at least on some platforms. — Additionally, deferred-length character strings and arrays are permitted since F2008 but not yet supported. --------------353CEAB19A42BF964D7B979E Content-Type: text/x-patch; charset="UTF-8"; name="optional-fix-v2.diff" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="optional-fix-v2.diff" Content-length: 6376 gcc/fortran/ * trans-openmp.c (gfc_omp_is_optional_argument, gfc_omp_check_optional_argument): Handle type(c_ptr),value which uses a hidden argument for the is-present check. gcc/ * omp-low.c (lower_omp_target): For use_device_ptr/use_derice_addr and Fortran's optional arguments, unconditionally add the is-present condition before the libgomp call. libgomp/ * testsuite/libgomp.fortran/use_device_ptr-optional-2.f90: Add 'type(c_ptr), value' test case. Conditionally map the per-value passed arguments. diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c index d9dfcabc65e..f21785fa8c3 100644 --- a/gcc/fortran/trans-openmp.c +++ b/gcc/fortran/trans-openmp.c @@ -60,7 +60,8 @@ gfc_omp_is_allocatable_or_ptr (const_tree decl) /* True if the argument is an optional argument; except that false is also returned for arguments with the value attribute (nonpointers) and for - assumed-shape variables (decl is a local variable containing arg->data). */ + assumed-shape variables (decl is a local variable containing arg->data). + Note that pvoid_type_node is for 'type(c_ptr), value. */ static bool gfc_omp_is_optional_argument (const_tree decl) @@ -68,6 +69,7 @@ gfc_omp_is_optional_argument (const_tree decl) return (TREE_CODE (decl) == PARM_DECL && DECL_LANG_SPECIFIC (decl) && TREE_CODE (TREE_TYPE (decl)) == POINTER_TYPE + && TREE_TYPE (decl) != pvoid_type_node && GFC_DECL_OPTIONAL_ARGUMENT (decl)); } @@ -99,9 +101,12 @@ gfc_omp_check_optional_argument (tree decl, bool for_present_check) || !GFC_DECL_OPTIONAL_ARGUMENT (decl)) return NULL_TREE; - /* For VALUE, the scalar variable is passed as is but a hidden argument - denotes the value. Cf. trans-expr.c. */ - if (TREE_CODE (TREE_TYPE (decl)) != POINTER_TYPE) + /* Scalars with VALUE attribute which are passed by value use a hidden + argument to denote the present status. They are passed as nonpointer type + with one exception: 'type(c_ptr), value' as '*void'. */ + /* Cf. trans-expr.c's gfc_conv_expr_present. */ + if (TREE_CODE (TREE_TYPE (decl)) != POINTER_TYPE + || TREE_TYPE (decl) == pvoid_type_node) { char name[GFC_MAX_SYMBOL_LEN + 2]; tree tree_name; diff --git a/gcc/omp-low.c b/gcc/omp-low.c index 19132f76da2..0e66a68ff36 100644 --- a/gcc/omp-low.c +++ b/gcc/omp-low.c @@ -11981,8 +11981,6 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx) case OMP_CLAUSE_USE_DEVICE_PTR: case OMP_CLAUSE_USE_DEVICE_ADDR: case OMP_CLAUSE_IS_DEVICE_PTR: - bool do_optional_check; - do_optional_check = false; ovar = OMP_CLAUSE_DECL (c); var = lookup_decl_in_outer_ctx (ovar, ctx); @@ -12004,10 +12002,7 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx) } type = TREE_TYPE (ovar); if (lang_hooks.decls.omp_array_data (ovar, true)) - { - var = lang_hooks.decls.omp_array_data (ovar, false); - do_optional_check = true; - } + var = lang_hooks.decls.omp_array_data (ovar, false); else if ((OMP_CLAUSE_CODE (c) == OMP_CLAUSE_USE_DEVICE_ADDR && !omp_is_reference (ovar) && !omp_is_allocatable_or_ptr (ovar)) @@ -12027,14 +12022,12 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx) && omp_is_allocatable_or_ptr (ovar)))) { var = build_simple_mem_ref (var); - do_optional_check = true; } var = fold_convert (TREE_TYPE (x), var); } } tree present; - present = (do_optional_check - ? omp_check_optional_argument (ovar, true) : NULL_TREE); + present = omp_check_optional_argument (ovar, true); if (present) { tree null_label = create_artificial_label (UNKNOWN_LOCATION); diff --git a/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90 b/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90 index 41abf17eede..e57abfbed5c 100644 --- a/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90 +++ b/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90 @@ -1,33 +1,47 @@ ! Check whether absent optional arguments are properly ! handled with use_device_{addr,ptr}. program main + use iso_c_binding, only: c_ptr, c_loc implicit none (type, external) call foo() contains - subroutine foo(v, w, x, y, z) + subroutine foo(v, w, x, y, z, cptr) integer, target, optional, value :: v integer, target, optional :: w integer, target, optional :: x(:) integer, target, optional, allocatable :: y integer, target, optional, allocatable :: z(:) + type(c_ptr), target, optional, value :: cptr integer :: d - !$omp target data map(d) use_device_addr(v, w, x, y, z) - if(present(v)) stop 1 - if(present(w)) stop 2 - if(present(x)) stop 3 - if(present(y)) stop 4 - if(present(z)) stop 5 + ! Need to map per-VALUE arguments, if present + if (present(v)) then + !$omp target enter data map(to:v) + stop 1 ! – but it shall not be present in this test case. + end if + if (present(cptr)) then + !$omp target enter data map(to:cptr) + stop 2 ! – but it shall not be present in this test case. + end if + + !$omp target data map(d) use_device_addr(v, w, x, y, z, cptr) + if (present(v)) then; v = 5; stop 1; endif + if (present(w)) then; w = 5; stop 2; endif + if (present(x)) then; x(1) = 5; stop 3; endif + if (present(y)) then; y = 5; stop 4; endif + if (present(z)) then; z(1) = 5; stop 5; endif + if (present(cptr)) then; cptr = c_loc(v); stop 6; endif !$omp end target data ! Using 'v' in use_device_ptr gives an ICE ! TODO: Find out what the OpenMP spec permits for use_device_ptr - !$omp target data map(d) use_device_ptr(w, x, y, z) - if(present(w)) stop 6 - if(present(x)) stop 7 - if(present(y)) stop 8 - if(present(z)) stop 9 + !$omp target data map(d) use_device_ptr(w, x, y, z, cptr) + if(present(w)) then; w = 5; stop 11; endif + if(present(x)) then; x(1) = 5; stop 12; endif + if(present(y)) then; y = 5; stop 13; endif + if(present(z)) then; z(1) = 5; stop 14; endif + if(present(cptr)) then; cptr = c_loc(x); stop 15; endif !$omp end target data end subroutine foo end program main --------------353CEAB19A42BF964D7B979E--