Hi, Sergey

please see my comments. Fixes applied and force-pushed to the branch.

Sergey

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.

Updated:

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.

replaced with Debug

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