Hi, Sergey please see my comments. Fixes applied and force-pushed to the branch. Sergey On 18.04.2024 11:24, Sergey Kaplun wrote: > Hi, Sergey > > On 16.04.24, Sergey Bronnikov wrote: >> Hi, Sergey >> >> On 12.04.2024 13:27, Sergey Kaplun wrote: > > >>>> 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. Detection steps moved to workflow. > >> >>> 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. >>> > > >>>> + # >>>> + # 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: --- a/.github/workflows/avx512-build-testing.yml +++ b/.github/workflows/avx512-build-testing.yml @@ -40,13 +40,11 @@ jobs:        - name: Detect a presence of AVX512          id: avx512_script          run: | -          # Set the avx512_support environment variable to '0' -          # when AVX512 is supported and '1' otherwise. -          # -          # Normally, `grep` has the exit status is 0 if a line is -          # selected, 1 if no lines were selected, and 2 if an error -          # occurred. -          avx512_support=$(grep avx512f /proc/cpuinfo 2>&1 > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1) +          # Set the avx512_support environment variable to an exit +          # code returned by `grep`. The exit status of `grep` is +          # 0 if lines were found, 1 if no lines were found, and 2 +          # if an error occurred. +          avx512_support=$(grep avx512f /proc/cpuinfo 2>&1 > /dev/null; echo $?)            echo "avx512_support=$avx512_support" >> $GITHUB_OUTPUT          shell: bash        - name: setup Linux > 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 "$?"? removed all "tricks" and left "echo $?" as requested >> 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. > > > >>>> + 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. > >> > > >>>> + 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? removed CMAKE_C_COMPILER at all >> 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. Moved. > >>>> 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 >>>