From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 82571 invoked by alias); 18 Dec 2018 09:35:46 -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 82554 invoked by uid 89); 18 Dec 2018 09:35:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.9 required=5.0 tests=BAYES_00,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=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; Tue, 18 Dec 2018 09:35:43 +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 1gZBmv-0002lP-GO from ChungLin_Tang@mentor.com for gcc-patches@gcc.gnu.org; Tue, 18 Dec 2018 01:35:41 -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; Tue, 18 Dec 2018 01:35:38 -0800 Reply-To: Subject: Re: [PATCH 2/6, OpenACC, libgomp] Async re-work, oacc-* parts To: Thomas Schwinge , Chung-Lin Tang CC: References: <12319572-dd02-c946-f2b9-9d047be9c707@mentor.com> From: Chung-Lin Tang Message-ID: <9c7ff0cd-b549-f012-55a3-8d67b07cd6e5@mentor.com> Date: Tue, 18 Dec 2018 09:35:00 -0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:60.0) Gecko/20100101 Thunderbird/60.3.3 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2018-12/txt/msg01281.txt.bz2 On 2018/12/17 9:52 PM, Thomas Schwinge wrote: > Hi Chung-Lin! > > On Fri, 14 Dec 2018 22:52:44 +0800, Chung-Lin Tang wrote: >> On 2018/12/14 10:17 PM, Thomas Schwinge wrote: >>> On Tue, 25 Sep 2018 21:10:47 +0800, Chung-Lin Tang wrote: >>>> --- a/libgomp/oacc-async.c >>>> +++ b/libgomp/oacc-async.c >>> >>>> +attribute_hidden struct goacc_asyncqueue * >>>> +lookup_goacc_asyncqueue (struct goacc_thread *thr, bool create, int async) >>>> +{ >>>> + /* The special value acc_async_noval (-1) maps to the thread-specific >>>> + default async stream. */ >>>> + if (async == acc_async_noval) >>>> + async = thr->default_async; >>>> + >>>> + if (async == acc_async_sync) >>>> + return NULL; >>>> + >>>> + if (async < 0) >>>> + gomp_fatal ("bad async %d", async); >>> >>> To make this "resolve" part more obvious, that is, the translation from >>> the "async" argument to an "asyncqueue" array index: >>> >>>> + if (!create >>>> + && (async >= dev->openacc.async.nasyncqueue >>>> + || !dev->openacc.async.asyncqueue[async])) >>>> + return NULL; >>>> +[...] >>> >>> ..., I propose adding a "async2id" function for that, and then rename all >>> "asyncqueue[async]" to "asyncqueue[id]". >> >> I don't think this is needed. This is the only place in the entire runtime that >> does asyncqueue indexing, adding more conceptual layers of re-directed indexing >> seems unneeded. > > It makes the code better understandable? Or, curious, why do you think > that the translation from an OpenACC async-argument to an internal > asyncqueue ID should not be a separate function? Because the index is (1) not used elsewhere; nor supposed to really, lookup_goacc_asyncqueue() is intended to be the centralized place for looking up async queues. and (2) the special async number case handling here is really short, creating another conceptual index-redirecting in the code feels like over-engineering. >> I do think the more descriptive comments are nice though. > > >>> And, this also restores the current trunk behavior, so that >>> "acc_async_noval" gets its own, separate "asyncqueue". >> >> Is there a reason we need to restore that behavior right now? > > Because otherwise that's a functional change ("regression") from the > current GCC trunk behavior, which I wouldn't expect in a re-work. Okay, but do take note that the acc_get/set_default_async is part of the upstreaming too. The behavior change is due to that new 2.5 functionality, not really because I arbitrarily changed things. Thanks, Chung-Lin