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

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

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/

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

<snipped>

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