From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121377 invoked by alias); 6 Dec 2018 14:20:12 -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 121353 invoked by uid 89); 6 Dec 2018 14:20:11 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=BAYES_00,GIT_PATCH_1,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=row, H*r:0800 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Dec 2018 14:20:09 +0000 Received: from svr-orw-mbx-02.mgc.mentorg.com ([147.34.90.202]) by relay1.mentorg.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-SHA384:256) id 1gUuVa-0007Fe-Dt from ChungLin_Tang@mentor.com ; Thu, 06 Dec 2018 06:20:06 -0800 Received: from [0.0.0.0] (147.34.91.1) by svr-orw-mbx-02.mgc.mentorg.com (147.34.90.202) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Thu, 6 Dec 2018 06:20:03 -0800 Reply-To: Subject: Re: [PATCH, OpenACC, 7/8] Multi-dimensional dynamic array support for OpenACC data clauses, libgomp support To: Jakub Jelinek , CC: , Thomas Schwinge References: <20181016131303.GZ11625@tucnak> From: Chung-Lin Tang Message-ID: Date: Thu, 06 Dec 2018 14:20:00 -0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:60.0) Gecko/20100101 Thunderbird/60.3.2 MIME-Version: 1.0 In-Reply-To: <20181016131303.GZ11625@tucnak> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2018-12/txt/msg00366.txt.bz2 Hi Jakub, thanks for the swift review a few weeks ago, and apologies I haven't been able to respond sooner. On 2018/10/16 9:13 PM, Jakub Jelinek wrote:>> +/* Dynamic array related data structures, interfaces with the compiler. */ >> + >> +struct da_dim { >> + size_t base; >> + size_t length; >> + size_t elem_size; >> + size_t is_array; >> +}; >> + >> +struct da_descr_type { >> + void *ptr; >> + size_t ndims; >> + struct da_dim dims[]; >> +}; > > Why do you call the non-contiguous arrays dynamic arrays? Is that some OpenACC term? > I'd also prefix those with gomp_ and it is important to make it clear what > is the ABI type shared with the compiler and what are the internal types. > struct gomp_array_descr would look more natural to me. Well it's not particularly an OpenACC term, just that non-contiguous arrays are often multi-dimensional arrays dynamically allocated and created through (arrays of) pointers. Are you strongly opposed to this naming? If so, I can adjust this part. I think the suggested 'gomp_array_descr' identifier looks descriptive, I'll revise that in an update, as well as add more comments to better describe its ABI significance with the compiler. >> + for (i = 0; i < mapnum; i++) >> + { >> + int kind = get_kind (short_mapkind, kinds, i); >> + if (GOMP_MAP_DYNAMIC_ARRAY_P (kind & typemask)) >> + { >> + da_data_row_num += gomp_dynamic_array_count_rows (hostaddrs[i]); >> + da_info_num += 1; >> + } >> + } > > I'm not really happy by adding several extra loops which will not do > anything in the case there are no non-contiguous arrays being mapped (for > now always for OpenMP (OpenMP 5 has support for non-contigious target update > to/from though) and guess rarely for OpenACC). > Can't you use some flag bit in flags passed to GOMP_target* etc. and do the > above loop only if the compiler indicated there are any? I originally strived to not have that loop, but because each row in the last dimension is mapped as its own target_var_desc, we need to count them at this stage to allocate the right number at start. Otherwise a realloc later seems even more ugly... We currently don't have a suitable flag word argument in GOMP_target*, GOACC_parallel*, etc. I am not sure if such a feature warrants changing the interface. If you are weary of OpenMP being affected, I can add a condition to restrict such processing to only (pragma_kind == GOMP_MAP_VARS_OPENACC). Is that okay? (at least before making any larger runtime interface adjustments) >> + tgt = gomp_malloc (sizeof (*tgt) >> + + sizeof (tgt->list[0]) * (mapnum + da_data_row_num)); >> + tgt->list_count = mapnum + da_data_row_num; >> tgt->refcount = pragma_kind == GOMP_MAP_VARS_ENTER_DATA ? 0 : 1; >> tgt->device_descr = devicep; >> struct gomp_coalesce_buf cbuf, *cbufp = NULL; > >> @@ -687,6 +863,55 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum, >> } >> } >> >> + /* For dynamic arrays. Each data row is one target item, separated from >> + the normal map clause items, hence we order them after mapnum. */ >> + for (i = 0, da_index = 0, row_start = 0; i < mapnum; i++) > > Even if nothing is in flags, you could just avoid this loop if the previous > loop(s) haven't found any noncontiguous arrays. I'll add a bit more checking to avoid these cases. Thanks, Chung-Lin