From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 3EEECA763C0; Thu, 18 Apr 2024 11:28:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3EEECA763C0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1713428930; bh=8OC91O7p/S+hkOXVn/5yjml9ZtYrwlME83fhlL1eUKM=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=bYKW7W7HYqyt2DSiaG0J+DqCo0c4hSAsK2NRwgmn4I6fvMG6XifAJYRCPXaINwyrZ ApQ1snUey0yci65kzA1JVOCjNoXyXZGxRdWxOKLRCNppEg21vCPFQ8O7jBFsFNZYl8 RhOAg7Z+gFvz9mibj9iDho3i05Hk62/wz/2Zqef0= Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [95.163.41.96]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C59E3A763C1 for ; Thu, 18 Apr 2024 11:28:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C59E3A763C1 Received: by smtp58.i.mail.ru with esmtpa (envelope-from ) id 1rxN8Q-0000000HSIx-2Byy; Thu, 18 Apr 2024 11:28:47 +0300 Date: Thu, 18 Apr 2024 11:24:42 +0300 To: Sergey Bronnikov Message-ID: References: <6777a43e3012332d04493f93d6afe7fb4156af1b.1712841312.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9F97E3C14763C38E25E23AD2F753BC837A560B79F28EE8E41182A05F5380850405D6DDFBB6782B69D89CEA9C911D09995C1B61FF30A167904472768C075C47EE6377EF39409332766 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F0ABDA2F087648F5EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006372338AE33E473C9B88638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D890339FF5DBB9CDAE5F12814CEA17BF1C1118A8A5198BE95FCC7F00164DA146DAFE8445B8C89999728AA50765F7900637CAEE156C82D3D7D9389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8062BEEFFB5F8EA3EF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE33AC447995A7AD183FC91FA280E0CE3D3A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735BA6625F88748EAEFC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5C7821E1E2DF4FF035002B1117B3ED696788FC3B76EC4CD65715D9AB585B0EB04823CB91A9FED034534781492E4B8EEAD9CFA8CFAC159CE19BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFC7D6D850539CD742B6C9E9756486AF2B1D67EBFAC0CF74226C80F26C6AC3D3F8C7C5EACBE3BE5B474AA6A4DA8F63241676052275FC2E37A205142C250FC8F7F357E17A50ACD4FB4DC226CC413062362A913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojHFx/al5BK5HXA2T/Mqt5fQ== X-DA7885C5: B3E372F9F833F227F255D290C0D534F9286859DFF134DA9008EB8C4F3F51354D8F65B0DD0A0B1BF75B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE3316C862827177E77519722EF389727343A9D5CB9CBC5063EAE49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/4][v2] ci: add a workflow for testing with AVX512 X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey On 16.04.24, Sergey Bronnikov wrote: > Hi, Sergey > > On 12.04.2024 13:27, Sergey Kaplun wrote: > >> diff --git a/.github/actions/setup-linux/action.yml b/.github/actions/setup-linux/action.yml > >> index f0171b83..19bdcfa2 100644 > >> --- a/.github/actions/setup-linux/action.yml > >> +++ b/.github/actions/setup-linux/action.yml > >> @@ -17,3 +17,15 @@ runs: > >> apt -y update > >> apt -y install cmake gcc make ninja-build perl > >> shell: bash > >> + - name: Detect a presence of AVX512 > > I suppose it shouldn't be a part of setup-linux action, > > It was requested by Max. And what is the rationale for it? Why don't use this detection during steps in our workflow? Hence, we avoid this unnecesary step for other jobs. > > > > but just a one > > step in testing-avx512.yml, see [1] for details of steps content usage. > > > > Is it possible or did I miss some GH-action API trick here? > > But anyway it should be done not in the same action. > > > > > >> + # > >> + # Normally `grep` has the exit status is 0 if a line is > > Typo: s/Normally/Normally,/ > > Also, "has the exit status is" has two verbs, so I suggest to > > reformulate this like the following: > > > > | Normally, the exit status of `grep` is 0 if a line is selected, 1 if > > | no lines were selected, and 2 if an error occurred. Ping. > > > >> + # selected, 1 if no lines were selected, and 2 if an error > >> + # occurred. > >> + avx512_support=$(grep avx /proc/cpuinfo 2>&1 > /dev/null; [[ $? == 0 ]] && echo 0 || echo 1) > > Why not just echo the "$?"? > because comment above. `grep` has three possible values. Yes, but why do we need such tricks if we are still interested in the 0 value only? Also, with this and the logging of the `avx512_support`, we may see the reason why the workflow was skipped: an incorrect `grep` command (for some reason) or a not-founded CPU feature flag. Hence, if the command fails with exit code 2, that means that our CI settings are incorrect and need to be adjusted. > > > > > >> + echo "avx512_support=$avx512_support" >> $GITHUB_OUTPUT > > Minor: It will be nice to echo some message about the value of this > > variable. Hence, we can check this from the GitHub logs. > > What for? Job will be skipped if there is no AVX512 support. > > What is the value of the message? See the rationale above. > > > > > >> + if: needs.avx512_script.outputs.avx512_support == 0 > >> + run: > > >> + cmake -S . -B ${{ env.BUILDDIR }} > >> + -G Ninja > >> + -DCMAKE_BUILD_TYPE=RelWithDebInfo > > Please add a comment. Why do we check the non-debug build here? > > If there is no reason for it, maybe it is better to check both: the > > debug and release builds within a simple matrix. > There is no reason for this. So, let's check the Debug build only. And with enabled assertions. They are useful for testing. > > > >> + -DCMAKE_C_FLAGS=-march=skylake-avx512 > >> + -DCMAKE_C_COMPILER=gcc > > Do we need to specify compiler manually? > Yes, CFLAG is gcc-specific. Emm, is it? | $ clang -march=skylake-avx512 /tmp/t.c -o /tmp/t.exe | $ clang --version | clang version 17.0.6 | Target: x86_64-pc-linux-gnu | Thread model: posix | InstalledDir: /usr/lib/llvm/17/bin | Configuration file: /etc/clang/x86_64-pc-linux-gnu-clang.cfg Maybe it is unsupported for clang version we used in CI? > > > >> + - name: build > >> + if: needs.avx512_script.outputs.avx512_support == 0 > >> + run: cmake --build ${{ env.BUILDDIR }} --parallel > >> + - name: run regression tests > >> + if: needs.avx512_script.outputs.avx512_support == 0 > >> + run: cmake --build ${{ env.BUILDDIR }} --parallel --target LuaJIT-test > >> diff --git a/test/LuaJIT-tests/lib/ffi/bit64.lua b/test/LuaJIT-tests/lib/ffi/bit64.lua > >> index d1b47bef..cf3a96eb 100644 > >> --- a/test/LuaJIT-tests/lib/ffi/bit64.lua > >> +++ b/test/LuaJIT-tests/lib/ffi/bit64.lua > >> @@ -41,7 +41,7 @@ do --- tobit/band assorted C types > >> end > >> end > >> > >> -do --- tobit/band negative unsigned enum > >> +do --- tobit/band negative unsigned enum -avx512 > > Should we also to set such variable in the CMakeLists.txt when we met > > the corresponding condition? > Yes, double check. I meant that part of the third commit should be done within this patch, as you've already fixed. > > > >> local x = ffi.new("uenum_t", -10) > >> local y = tobit(x) > >> local z = band(x) > >> -- > >> 2.34.1 > > [1]:https://docs.github.com/en/actions/learn-github-actions/contexts#example-usage-of-the-steps-context > > [2]:https://www.laruence.com/x86/VCVTTSD2USI.html > > -- Best regards, Sergey Kaplun