From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.130]) by sourceware.org (Postfix) with ESMTPS id 4A8133858C60 for ; Sat, 6 Jan 2024 19:45:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4A8133858C60 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=net-b.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=net-b.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 4A8133858C60 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=212.227.126.130 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704570352; cv=none; b=EEXfSskCyT31JHgc++y1rM0EE0UvAX69r0rzLDTt1Qpf+zwoTTMin8izRsuQKe2Skwn274qRj0Z/mHH5I3OmeMedSr5ZrsCsbCpnwLNUa0aIY1lF/H2Wp94WbftYG+imJjEZo70C5SjvLlSrRSHdNZckySeCmauzQz97B0FA9KQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704570352; c=relaxed/simple; bh=CfDkFNh2tJcOj3i34TGrjGwcfAa4UU+NAzFSlQ3TNPw=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=O1eciTfl7SlcjE7WqrOXjpoLrFlAMGA5PH3J1U7oIobJWAIWdUvud3X/MQO5XaVhc1LeaITjf1lNMYYSBn+rhtaxtldJeM2O9abZGFSk12s9puBzSKXHUobz7rrqd50gyJFneM4ipg84EDw123VGsStsVA2YqQ9dtK5+Mz7xezo= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [192.168.0.26] ([89.246.122.167]) by mrelayeu.kundenserver.de (mreue009 [213.165.67.97]) with ESMTPSA (Nemesis) id 1MlfCk-1qwBS10pSE-00inIJ; Sat, 06 Jan 2024 20:45:43 +0100 Message-ID: Date: Sat, 6 Jan 2024 20:45:42 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/8] OpenMP: metadirective tree data structures and front-end interfaces Content-Language: en-US To: Sandra Loosemore , gcc-patches@gcc.gnu.org Cc: kcy@codesourcery.com, tobias@codesourcery.com, jakub@redhat.com References: <20240106185257.126445-1-sandra@codesourcery.com> <20240106185257.126445-2-sandra@codesourcery.com> From: Tobias Burnus In-Reply-To: <20240106185257.126445-2-sandra@codesourcery.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K1:3tg4WeF+WaoyoHDWQUF70drWUtZNoGXE97iNjmA1c+1RfOy7ZlV oM/VHi9JCK8omJ+yznMPxfkM3EQG1ZavZvILCzrb032iYiB3BBRQ/PPjwzzkZ+YDl80+BKj hXjygdeaNK74qQ1QZZxTTJCfYwpUvfcFarV3XyoN1clSyAuhLc4OMP81u94JmzYzq9JJIL7 ygYckTxozixfuWBBmwfhw== UI-OutboundReport: notjunk:1;M01:P0:WI/2+UXuazU=;asNA7uI+w+bt2tdZca8wipnW9cr sppf4QWCy0mxhe5tCBsXgqRMoRSAUuhtg6rn5s5m41odr1WeMdkZTV0oFU1MzqP5+31Z8jNwZ VY7hEJJbgjHRzAZiQeoKjbzSb3mCIDhMqnzjafzigCn/LljAROvU/iiUxRchfMQN/IKqqgxWT i4urlqpAGEWNAgFdprHlLjPBo7YDPlN9XB8zBQF3jlYrWYmLNis4qr2bbHrVnJn06L2O3mefK uEEISaCgeI0r/IzUmEDmu5vST+O4UD1DkU241h6rqf4Yi0LOLmqyXSyxiBRmfYEpInABcojRK HkHxCPIDRA3kU4SbW2D/2YYsYKbDg+lNjJ6XaWjLmijxUTiuAsDZADPi0aZszjyXA9flP26lz Vl2NgXKgmYNZXZSpNZu4EjGC4ulsf85h0UrM3JTQUCJdpfvOOEiyzZNxo77vp8G1bOOvZLueO IXo5TdoymBMCq5ia6muNvlXR3FEA2icnzreIkYw6j/piQ3SiFS4CLOHPdiWU67Znrho0XD8HI xPYPyJg5iT+2M8VSjbMUj2Bud9TnBCCOCs/o9M042vlrHkgeRVbyWSah6o1EInC0cT7qiqs23 aK06quOAYBXjpP6D5cwYkJ2IIRcoNTGGoh0MZiXCoOvkwQCCUCee3XhgxH7f1DTW7EzMoVogn MVGqq3tVen2mBXZc7eeo15183hKVimRPawWqDiRqsClDuLWqJTQdYTHveDiUsLkVTEzQ+87Vc wY3gwlQdDybO/vwThh+mVxLRAJqrAL24QHlbSTWjr3tLuvTt2dRdLX1uIWYoI6BGEUduovJkY sumVqYkZzI8GqIg2WFdp36JK2f0teaDqe3RKyFZtyvmZnbevaKvGDAsvG6MFbivAI9aDjLbtS HZLqkyBo5QVXz8Q== X-Spam-Status: No, score=-1.5 required=5.0 tests=BAYES_00,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Sandra, given that it is "just" a revised patch of previously posted patch and (code wise not file wise) very localized to OpenMP code - and even there touching only 'declare ...' code in a simple way: I would be still fine to have it in GCC 14, but I want to give Jakub a chance to shout... Glancing at the code, it looks okayish, but I have two quick comments. (I have now an errant to do - and will continue later with the review.) Sandra Loosemore wrote: > +static tree > +omp_dynamic_cond (tree ctx) > +{ > + tree expr = NULL_TREE; > + > + tree user = omp_get_context_selector (ctx, OMP_TRAIT_SET_USER, > + OMP_TRAIT_USER_CONDITION); > + if (user) > + { > + tree expr_list = OMP_TS_PROPERTIES (user); > + > + gcc_assert (OMP_TP_NAME (expr_list) == NULL_TREE); > + > + /* The user condition is not dynamic if it is constant. */ > + if (!tree_fits_shwi_p (TREE_VALUE (expr_list))) > + expr = TREE_VALUE (expr_list); > + } > + > + tree target_device > + = omp_get_context_selector_list (ctx, OMP_TRAIT_SET_TARGET_DEVICE); > + if (target_device) > + { > + tree device_num = null_pointer_node; > + tree kind = null_pointer_node; > + tree arch = null_pointer_node; > + tree isa = null_pointer_node; > + > + tree device_num_sel > + = omp_get_context_selector (ctx, OMP_TRAIT_SET_TARGET_DEVICE, > + OMP_TRAIT_DEVICE_NUM); > + if (device_num_sel) > + device_num = OMP_TP_VALUE (OMP_TS_PROPERTIES (device_num_sel)); > + else > + device_num = build_int_cst (integer_type_node, -1); I think this won't correctly distinguish 'omp_initial_device' (= -1) from unset. OpenMP has 'omp_invalid_device', 'omp_initial_device' and then the values 0 to omp_get_num_devices() [where the latter denotes the host, 0 ... < omp_get_num_device() are non-host devices]. > /* Compute *SCORE for context selector CTX. Return true if the score > would be different depending on whether it is a declare simd clone or > @@ -2194,12 +2308,21 @@ omp_context_compute_score (tree ctx, score_wide_int *score, bool declare_simd) > { > tree selectors > = omp_get_context_selector_list (ctx, OMP_TRAIT_SET_CONSTRUCT); > - bool has_kind = omp_get_context_selector (ctx, OMP_TRAIT_SET_DEVICE, > - OMP_TRAIT_DEVICE_KIND); > - bool has_arch = omp_get_context_selector (ctx, OMP_TRAIT_SET_DEVICE, > - OMP_TRAIT_DEVICE_ARCH); > - bool has_isa = omp_get_context_selector (ctx, OMP_TRAIT_SET_DEVICE, > - OMP_TRAIT_DEVICE_ISA); > + bool has_kind > + = (omp_get_context_selector (ctx, OMP_TRAIT_SET_DEVICE, > + OMP_TRAIT_DEVICE_KIND) > + || omp_get_context_selector (ctx, OMP_TRAIT_SET_TARGET_DEVICE, > + OMP_TRAIT_DEVICE_KIND)); I do not completely understand how that's used, but I have the feeling that having a match that contains both 'device' and 'target_device' is not correctly handled here. > int *scores > = (int *) alloca ((2 * nconstructs + 2) * sizeof (int)); That's not new but I have the feeling it should be '+ 3' and not '+ 2' for device or target_device + and having both device and target device, it might even need to be + 6. I also wonder whether 'alloca' will really work - or only when inlined at all call sites. (Currenlty, it is called 6 times - and a 7th time with this patch.) That's how far I came and before having to leave for now. (And having no time to think about what I wrote about 'score'.) Tobias