Hi, Sergey

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,

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/

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

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?

+      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