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 8F49FB5FCC0; Fri, 12 Apr 2024 13:31:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8F49FB5FCC0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1712917904; bh=s/2YObId0Zl4sCOMqnaJbG1igqJA4g7jGM4xyLuq+08=; 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=OqZJg1NnNWRMB9KDIJKplM6d/Q5ajiaCfiISRZw9WUZqmv5xNgs997PqbTVSjWy8V yTv1UG88eJqFa0MlRlpFoJksh5UrVSwkDbT5KpJWexlrfb3pR+brpflbJBs69XP+vP ijuuRpvvRslLPXgRbp3xG11G7ct8RNmCokvYNA+U= Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [95.163.41.66]) (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 900B3B5FCC0 for ; Fri, 12 Apr 2024 13:31:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 900B3B5FCC0 Received: by smtp43.i.mail.ru with esmtpa (envelope-from ) id 1rvEC5-000000078PO-3G5Q; Fri, 12 Apr 2024 13:31:42 +0300 Date: Fri, 12 Apr 2024 13:27:40 +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: <6777a43e3012332d04493f93d6afe7fb4156af1b.1712841312.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD91C726AF9233A4C6FE79860C6293C741DABECEEFF07AE47041313CFAB8367EF908E2BE116634AD74DFD6BCEB376527FA996250F415FCC1BCB8A080EF6628F9DCBDBA66081925E5CC6F253FDB7F52A2E4C X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7089AE85CCA60F323EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B3D52627AD81B52CEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B73AB1701401CD8713A93B5BE11B76F8D118333E814D9E9FD864B6505D524E020A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3ED8438A78DFE0A9E117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CD2DCF9CF1F528DBC2E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89FB26E97DCB74E62526D8C47C27EEC5E9FB5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A56C3593D462FCC7725002B1117B3ED6960066694B64DDE057B74D9144D44E4FCF823CB91A9FED034534781492E4B8EEADB1D70E2111C441FFBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFCD9D578115A94E3AE3F086306CE1DBEEAF4BBD8794CCA3EF1CAEFD7A2E38CD9E66971B16C155F0C91E5F415AA55898CC62BEEBF0502E496545E91FD5AB84A7AFD272558F54EA26DEC226CC413062362A913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojj8Sl06kWf00mrr9k4k71ww== X-DA7885C5: 0004795CB3E52835F255D290C0D534F95FBE0F56F302B1A6FABEC64A6CD721F9E3F2CDC5B605CE805B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE33BD339E5402BA4454FC56043B428257731D1F338658511B0BE49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F 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: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the patch! Please consider my comments below. On 11.04.24, Sergey Bronnikov wrote: > From: Sergey Bronnikov > > This commit adds a workflow for building and testing with enabled > AVX512. Typo: s/with enabled AVX512/with AVX512 enabled/ > > Needed for tarantool/tarantool#9595 > Related to tarantool/tarantool#6787 Typo: s/Related/Relates/ | git log | grep Relates | wc -l | 29 | git log | grep Related | wc -l | 7 > --- > .github/actions/setup-linux/action.yml | 12 +++++ > .github/workflows/avx512-build-testing.yml | 54 ++++++++++++++++++++++ > test/LuaJIT-tests/lib/ffi/bit64.lua | 2 +- > 3 files changed, 67 insertions(+), 1 deletion(-) > create mode 100644 .github/workflows/avx512-build-testing.yml > > 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, 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. > + id: avx512_script > + run: | > + # Set avx512_support environment variable to 'true' when AVX512 Typo: s/avx512_support/the avx512_support/ > + # is supported and 'false' otherwise. The comment is a little bit misleading since the resulting values are "0" or "1". Changing it to 'true' or 'false' looks like a good idea and simplifies reading later. > + # > + # 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. > + # 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 "$?"? On my laptop without avx512 support this command returns true. | $ grep avx /proc/cpuinfo 2>&1 > /dev/null; [[ $? == 0 ]] && echo 0 || echo 1 | 0 I have no avx512 support, but avx|avx2 support. Hence, there is matching content: | $ grep -o -P '.{4}avx.{4}' /proc/cpuinfo | ave avx f16 | mi1 avx2 sm | ave avx f16 | mi1 avx2 sm | ave avx f16 | mi1 avx2 sm | ave avx f16 | mi1 avx2 sm I suppose we should check for the "AVX512F" CPUID feature flag: its supports allows to emit vcvttsd2usi [2] instruction that causes troubles. > + 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. Side note: Do I understand correctly that GH-actions output is always a string that evaluates to `true` regardless of its content? > + shell: bash > diff --git a/.github/workflows/avx512-build-testing.yml b/.github/workflows/avx512-build-testing.yml > new file mode 100644 > index 00000000..d70fa661 > --- /dev/null > +++ b/.github/workflows/avx512-build-testing.yml > + > +jobs: > + test-avx512: Minor: I suggest adding a name for the job to be consistent with other jobs. Something like "LuaJIT avx512"" > + strategy: > + fail-fast: false > + runs-on: [self-hosted, regular, x86_64, Linux] > + steps: > + - uses: actions/checkout@v4 > + with: > + fetch-depth: 0 > + submodules: recursive > + - name: setup Linux > + uses: ./.github/actions/setup-linux > + - name: configure > + 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. > + -DCMAKE_C_FLAGS=-march=skylake-avx512 > + -DCMAKE_C_COMPILER=gcc Do we need to specify compiler manually? > + - 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? > 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