From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id 7ED283858C78 for ; Tue, 12 Dec 2023 16:17:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7ED283858C78 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 7ED283858C78 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=68.232.129.153 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702397876; cv=none; b=ECCBEvs4bsM/SlfjzYN6M9N722WILj91v+rrwh2D00S12nY2eo4t+84zh8Ec43ZypJzL6uNzogpzXqGHTzkdqdmAt4s28/5qGcKdtmre7dAyyQZAQFlBCdfKZERpIk/PpwNkcmzLo70+XAeCZgYS5fqtWG/wf+pkCmg2iVGXTuQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702397876; c=relaxed/simple; bh=J0kx8XLDcn4k6RqG6INhWsk4Hrp5Rpip+/Mj0QTmCEQ=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=jJ1RK5iouB7GSzHr+Oss4/ygqNdFJhDsdaRUIfTmcit9wkMcl2K6I8cmemQLxhC8pUUYS+whVkXV0pU+oyWsyJuw5KgMp8tkSpxsCVPjISa1fbZPjd+pt81pRlPMJh7TjJZTzVH4DXVkLHwnoIRq3O4+pw5s5RDx53imUlu64Ek= ARC-Authentication-Results: i=1; server2.sourceware.org X-CSE-ConnectionGUID: Lo0yNT/GQNaUzVQ9Wkao9A== X-CSE-MsgGUID: 083efqEiTsW1cD2wY9eZIw== X-IronPort-AV: E=Sophos;i="6.04,270,1695715200"; d="scan'208";a="28372151" Received: from ies-gwy-02-in.mentorg.com ([192.94.31.182]) by esa1.mentor.iphmx.com with ESMTP; 12 Dec 2023 08:17:52 -0800 IronPort-SDR: Im3viQu2RlO+GlndWvAaPJ5YjfgmdVYPnMLWo1CSvyJBXNXje8Gf2FOoVIZdcF1mJTR0NtzomQ BrMemqTqzfEFoGUz6cyj7kVgBpNYJTQn8u+kwFdYvmHjrYn/5rZaskXP9BFlf5LtN6uOFREquG jPIa7Z7VhNRYlM4sPUbUHYGyrtyJZAoiJNqpDJeaZZ2vSqQlhOEaoOy9C1uUICPGHLn0xo1vpn CzK1AVGn/qZnAfEkX1J3CCa/Wx0xrhCG6ft+maRybIBuW6IT0pWrhh/7R8QR7nqR2RuMNztyXk cig= Message-ID: Date: Tue, 12 Dec 2023 16:17:47 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 2/6] libgomp, openmp: Add ompx_pinned_mem_alloc Content-Language: en-GB To: Tobias Burnus , References: <20231211170405.2538247-1-ams@codesourcery.com> <20231211170405.2538247-3-ams@codesourcery.com> <587963ae-266b-41ea-be78-a3170972d068@codesourcery.com> From: Andrew Stubbs In-Reply-To: <587963ae-266b-41ea-be78-a3170972d068@codesourcery.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-13.mgc.mentorg.com (139.181.222.13) To svr-ies-mbx-11.mgc.mentorg.com (139.181.222.11) X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 12/12/2023 10:05, Tobias Burnus wrote: > Hi Andrew, > > On 11.12.23 18:04, Andrew Stubbs wrote: >> This creates a new predefined allocator as a shortcut for using pinned >> memory with OpenMP.  The name uses the OpenMP extension space and is >> intended to be consistent with other OpenMP implementations currently in >> development. > > Discussed this with Jakub - and 9 does not permit to have a contiguous > range of numbers if OpenMP ever extends this, > > Thus, maybe start the ompx_ with 100. These numbers are not defined in any standard, are they? We can use whatever enumeration we choose. I'm happy to change them, but the *_mem_alloc numbers are used as an index into a constant table to map them to the corresponding *_mem_space, so do we really want to make it a sparse table? > We were also pondering whether it should be ompx_gnu_pinned_mem_alloc or > ompx_pinned_mem_alloc. It's a long time ago now, and I'm struggling to remember, but I think those names were agreed with some other parties (can't remember who though, and I may be thinking of the ompx_unified_shared_mem_alloc that is still to come). > The only other compiler supporting this flag seems to be IBM; their > compiler uses ompx_pinned_mem_alloc with the same meaning: > https://www.ibm.com/support/pages/system/files/inline-files/OMP5_User_Reference.pdf > (page 5) > > As the obvious meaning is what both compilers have, I am fine without > the "gnu" infix, which Jakub accepted. Good. > > * * * > > And you have not updated the compiler itself to support more this new > allocator. Cf. > > https://github.com/gcc-mirror/gcc/blob/master/gcc/testsuite/c-c++-common/gomp/allocate-9.c#L23-L28 > > That file gives an overview what needs to be changed: > > * The check functions mentioned there (seemingly for two ranges now) > > * Update the OMP_ALLOCATOR env var parser in env.c > > * That linked testcase (and possibly some some more) should be updated, > also to ensure that the new allocator is accepted + to check for new > unsupported values (99, 101 ?) > > If we now leave gaps, the const_assert in libgomp/allocator.c probably > needs to be updated as well. > > * * * > > Glancing through the patches, for test cases, I think you should > 'abort()' in CHECK_SIZE if it fails (rlimit issue or not supported > system). Or do you think that the results are still could make sense > when continuing and possibly failing later? Those were not meant to be part of the test, really, but rather to demystify failures for future maintainers. > > Tobias Thanks for the review. Andrew