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 040D9CE8A24; Tue, 16 Apr 2024 14:53:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 040D9CE8A24 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1713268430; bh=4kGVaGTHtxUWkhEHDNV/He5ChfuecwnNQPa29rr1Ny0=; 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=LcEkoLwFrMQdTAeoeWoSUFC4EXffCdo+CaXpbNLQ8NHco1JOfK/zSLi5L4GCK2081 NXVDclJTYnR+qsFmcTcLMR2uK0gF6lgSqwuT7CBzoKKbTvfGuf7fqo1ppa3uQFmtEA wnZhD6hqb1xezlNgfeMBAEuw5De4O/r+/1TnLjso= Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [95.163.41.94]) (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 86FA2CE8A24 for ; Tue, 16 Apr 2024 14:53:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 86FA2CE8A24 Received: by smtp56.i.mail.ru with esmtpa (envelope-from ) id 1rwhNi-00000006hRZ-3U73; Tue, 16 Apr 2024 14:53:47 +0300 Content-Type: multipart/alternative; boundary="------------yfneE0hyK7sLVBgXQYTDNXbh" Message-ID: Date: Tue, 16 Apr 2024 14:53:46 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun , Sergey Bronnikov References: <6777a43e3012332d04493f93d6afe7fb4156af1b.1712841312.git.sergeyb@tarantool.org> Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD91C726AF9233A4C6FA8143870728B8272760BE0FCA97B72EC1313CFAB8367EF908E2BE116634AD74D658D1C510BD285867FBBE522E338CBBD85D245E2D80D0E1CFB1BC3D001D4AD63DAEE934B56E2D9F8 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70F9B92A9B7E6C377EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375121C8F3070B83748638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8DF71E5185D418D4903A97AF91BD6A83B58182214C7AE0053CC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8062BEEFFB5F8EA3EF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C6089696B24BB1D19302FCEF25BFAB3454AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C34CF1817FC635140EBA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CFE478A468B35FE7671DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3F8A0C55DE5E8D0B235872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5C1DC089476D2B3A75002B1117B3ED69605B1B04447606ED503803A57F48E4E5A823CB91A9FED034534781492E4B8EEADAE4FDBF11360AC9BBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF8E62FA13B8D0DCB53DC30ACBC8CB64DDDAFC9C8563347ABA757CA516A6978BD6329B3AFCBA1AB93DC84F44966DD9651BB519B98BB316678A1EFE5E5208A9B7AD696D7DC2B4C335C3C226CC413062362A913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojwhzzK0WJvz3FQqRxz6V6Hg== X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B593071F05DA393C73C2EB5D77EF37489D121F698E4F2DE9296B7CBEF92542CD7C8795FA72BAB74744FC77752E0C033A69EA16A481184E8BB1C9B38E6EA4F046BE03A5DB60FBEB33A8A0DA7A0AF5A3A8387 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------yfneE0hyK7sLVBgXQYTDNXbh Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey On 12.04.2024 13:27, Sergey Kaplun wrote: > 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/ Fixed. > >> Needed for tarantool/tarantool#9595 >> Related to tarantool/tarantool#6787 > Typo: s/Related/Relates/ Fixed. > > | 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, It was requested by Max. > 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/ Fixed. >> + # 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. Fixed. > >> + # >> + # 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 "$?"? because comment above. `grep` has three possible values. > > 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. Changed to AVX512F. > >> + 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? > > Side note: Do I understand correctly that GH-actions output is always a > string that evaluates to `true` regardless of its content? 0/1 > >> + 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"" Added. > >> + 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. There is no reason for this. > >> + -DCMAKE_C_FLAGS=-march=skylake-avx512 >> + -DCMAKE_C_COMPILER=gcc > Do we need to specify compiler manually? Yes, CFLAG is gcc-specific. > >> + - 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. > >> 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 > --------------yfneE0hyK7sLVBgXQYTDNXbh Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey

On 12.04.2024 13:27, Sergey Kaplun wrote:
Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 11.04.24, Sergey Bronnikov wrote:

From: Sergey Bronnikov <sergeyb@tarantool.org>

This commit adds a workflow for building and testing with enabled
AVX512.

Typo: s/with enabled AVX512/with AVX512 enabled/
Fixed.


Needed for tarantool/tarantool#9595
Related to tarantool/tarantool#6787

Typo: s/Related/Relates/
Fixed. 

| 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,

    

    
It was requested by Max.

    

    


    

    

      
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/


    

    Fixed.

    

      

      

        
+        # 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.

    

    Fixed.

    

      



      

        
+        #
+        # 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 "$?"?

    

    because comment above. `grep` has three possible values.

    

      

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.

    

    Changed to AVX512F.

    

      



      

        
+        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?

    


    

    

      

Side note: Do I understand correctly that GH-actions output is always a
string that evaluates to `true` regardless of its content?

    

    0/1

    

      



      

        
+      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


      

      
<snipped>



      

        
+
+jobs:
+  test-avx512:


      

      
Minor: I suggest adding a name for the job to be consistent with other
jobs. Something like "LuaJIT avx512""

    

    Added.

    

      



      

        
+    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.

    

    There is no reason for this.

    

      



      

        
+          -DCMAKE_C_FLAGS=-march=skylake-avx512
+          -DCMAKE_C_COMPILER=gcc


      

      
Do we need to specify compiler manually?

    

    Yes, CFLAG is gcc-specific.

    

      



      

        
+      - 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.

    

      



      

        
   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



    

  


--------------yfneE0hyK7sLVBgXQYTDNXbh--