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

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.

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

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

> >
> >> +          -DCMAKE_C_FLAGS=-march=skylake-avx512
> >> +          -DCMAKE_C_COMPILER=gcc
> > Do we need to specify compiler manually?
> 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.

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