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
>