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 30858C236C8; Fri, 14 Jun 2024 18:24:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 30858C236C8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1718378657; bh=9zdDeW3kOpt8FrxSAo0lGylFHP7BuFbEpMThsJGPKZM=; 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=GjG+cVPkSTXghd+C7LyjRqAqpiKOVBlIrJHmpFcsbuUKSMzFXPaYWSmwTxgJBzEih 3lvioRYMvPZrdzRa3EYu9w9CYyWJb6X9qq+8kOgp93b03LuS7w1XRicDBbuyVaqsvf nG9E2jMXpttw6Rv82x49rrYMOFftP3qthpV4rtXk= 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 D50FF5BFDB6 for ; Fri, 14 Jun 2024 18:24:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D50FF5BFDB6 Received: by smtp58.i.mail.ru with esmtpa (envelope-from ) id 1sI8mk-0000000AjXL-2E0y; Fri, 14 Jun 2024 18:24:15 +0300 Content-Type: multipart/alternative; boundary="------------tOPk30sa06x0Cjma2dgxuoVJ" Message-ID: Date: Fri, 14 Jun 2024 18:24:14 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun References: <6777a43e3012332d04493f93d6afe7fb4156af1b.1712841312.git.sergeyb@tarantool.org> In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9AC8CA0B4439200FAA6268B5D05BFD5F405B302337288676B00894C459B0CD1B92E171CDF7E690699273326E884B17ED6E06E9553464A238BB84981FF7480A3BBFB652AD2CEFA51F0 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7EC0B1A4921CAE631EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063721BEEAF38C6AE0B78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D838D74CEBBA64E07C651579382E55F503072EB9AAAB1F6CD2CC7F00164DA146DAFE8445B8C89999728AA50765F7900637CAEE156C82D3D7D9389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8989FD0BDF65E50FBF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CBC0ADEB1C81BB3622D242C3BD2E3F4C64AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C399B6D7FD073FD0EFBA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE74F0F518E68DBD4F843847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5486568C6E86C9FEE5002B1117B3ED696DBE67DBDC8268BB0715D9AB585B0EB04823CB91A9FED034534781492E4B8EEADA3A806F356AF31D6 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF31C2EC8CE3B82E9CCE9FD0DCB8206C9DD4835BD1AEB4F654232DCF58A43888DDA5CD7B9A90500322BC11D93875BCBCACAD6BFCAFD73D978785CAB63D238EE0379F2D1F32013C05ADC226CC413062362A913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojqIk+/laaskDQpbcrDsVzxg== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D61402E171CDF7E690699273326E884B17ED656CC109BFAF9B60E0152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 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: Sergey Bronnikov , 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. --------------tOPk30sa06x0Cjma2dgxuoVJ Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Sergey please see my comments. Fixes applied and force-pushed to the branch. Sergey On 18.04.2024 11:24, Sergey Kaplun wrote: > 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. Detection steps moved to workflow. > >> >>> 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. Updated: --- a/.github/workflows/avx512-build-testing.yml +++ b/.github/workflows/avx512-build-testing.yml @@ -40,13 +40,11 @@ jobs:        - name: Detect a presence of AVX512          id: avx512_script          run: | -          # Set the avx512_support environment variable to '0' -          # when AVX512 is supported and '1' otherwise. -          # -          # Normally, `grep` has the exit status is 0 if a line is -          # selected, 1 if no lines were selected, and 2 if an error -          # occurred. -          avx512_support=$(grep avx512f /proc/cpuinfo 2>&1 > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1) +          # Set the avx512_support environment variable to an exit +          # code returned by `grep`. The exit status of `grep` is +          # 0 if lines were found, 1 if no lines were found, and 2 +          # if an error occurred. +          avx512_support=$(grep avx512f /proc/cpuinfo 2>&1 > /dev/null; echo $?)            echo "avx512_support=$avx512_support" >> $GITHUB_OUTPUT          shell: bash        - name: setup Linux > 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 "$?"? removed all "tricks" and left "echo $?" as requested >> 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. replaced with Debug > >>>> + -DCMAKE_C_FLAGS=-march=skylake-avx512 >>>> + -DCMAKE_C_COMPILER=gcc >>> Do we need to specify compiler manually? removed CMAKE_C_COMPILER at all >> 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. Moved. > >>>> 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 >>> --------------tOPk30sa06x0Cjma2dgxuoVJ Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Sergey


please see my comments. Fixes applied and force-pushed to the branch.

Sergey

On 18.04.2024 11:24, Sergey Kaplun wrote:
Hi, Sergey

On 16.04.24, Sergey Bronnikov wrote:

Hi, Sergey

On 12.04.2024 13:27, Sergey Kaplun wrote:

<snipped>


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.
Detection steps moved to workflow.






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.


<snipped>



          

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


        

      

    

    
Updated:

    --- a/.github/workflows/avx512-build-testing.yml

    +++ b/.github/workflows/avx512-build-testing.yml

    @@ -40,13 +40,11 @@ jobs:

           - name: Detect a presence of AVX512

             id: avx512_script

             run: |

    -          # Set the avx512_support environment variable to '0'

    -          # when AVX512 is supported and '1' otherwise.

    -          #

    -          # Normally, `grep` has the exit status is 0 if a line is

    -          # selected, 1 if no lines were selected, and 2 if an
    error

    -          # occurred.

    -          avx512_support=$(grep avx512f /proc/cpuinfo 2>&1
    > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1)

    +          # Set the avx512_support environment variable to an exit

    +          # code returned by `grep`. The exit status of `grep` is

    +          # 0 if lines were found, 1 if no lines were found, and 2

    +          # if an error occurred.

    +          avx512_support=$(grep avx512f /proc/cpuinfo 2>&1
    > /dev/null; echo $?)

               echo "avx512_support=$avx512_support" >>
    $GITHUB_OUTPUT

             shell: bash

           - name: setup Linux

    

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


        

      

    

    removed all "tricks" and left "echo $?" as requested

    

      

        
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.



      

        

          

        

      

      
<snipped>



      

        

          

          

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



      

        



        

          

        

      

      
<snipped>



      

        

          

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

    

    
replaced with Debug 

    

    


    

    

      



      

        

          

          

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


          

          
Do we need to specify compiler manually?

        

      

    

    removed CMAKE_C_COMPILER at all

    

      

        

          

        

        
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.

    

    Moved.

    

      



      

        

          

          

            
    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



        

      

      

    

  
  


--------------tOPk30sa06x0Cjma2dgxuoVJ--