Re: [Tarantool-patches] [PATCH luajit 1/4][v2] ci: add a workflow for testing with AVX512

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 6336 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 12116 bytes --]