<!DOCTYPE html>
<html data-lt-installed="true">
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body style="padding-bottom: 1px;">
    <p>Hi, Sergey</p>
    <p><br>
    </p>
    <p>please see my comments. Fixes applied and force-pushed to the
      branch.</p>
    <p>Sergey<br>
    </p>
    <div class="moz-cite-prefix">On 18.04.2024 11:24, Sergey Kaplun
      wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:ZiDYylFah6xu6LXk@root">
      <pre class="moz-quote-pre" wrap="">Hi, Sergey

On 16.04.24, Sergey Bronnikov wrote:
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Hi, Sergey

On 12.04.2024 13:27, Sergey Kaplun wrote:
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
<snipped>

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">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
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">I suppose it shouldn't be a part of setup-linux action,
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
It was requested by Max.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
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.</pre>
    </blockquote>
    Detection steps moved to workflow.<br>
    <p><span class="HwtZe" lang="en"><span class="jCAhz JxVs2d ChMk0b"><span
            class="ryNqvb"><br>
          </span></span></span></p>
    <p><span class="HwtZe" lang="en"><span class="jCAhz JxVs2d ChMk0b"><span
            class="ryNqvb"></span></span></span></p>
    <blockquote type="cite" cite="mid:ZiDYylFah6xu6LXk@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">

</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">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.

</pre>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
<snipped>

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+        #
+        # Normally `grep` has the exit status is 0 if a line is
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">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.
</pre>
        </blockquote>
      </blockquote>
    </blockquote>
    <p>Updated:</p>
    --- a/.github/workflows/avx512-build-testing.yml<br>
    +++ b/.github/workflows/avx512-build-testing.yml<br>
    @@ -40,13 +40,11 @@ jobs:<br>
           - name: Detect a presence of AVX512<br>
             id: avx512_script<br>
             run: |<br>
    -          # Set the avx512_support environment variable to '0'<br>
    -          # when AVX512 is supported and '1' otherwise.<br>
    -          #<br>
    -          # Normally, `grep` has the exit status is 0 if a line is<br>
    -          # selected, 1 if no lines were selected, and 2 if an
    error<br>
    -          # occurred.<br>
    -          avx512_support=$(grep avx512f /proc/cpuinfo 2>&1
    > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1)<br>
    +          # Set the avx512_support environment variable to an exit<br>
    +          # code returned by `grep`. The exit status of `grep` is<br>
    +          # 0 if lines were found, 1 if no lines were found, and 2<br>
    +          # if an error occurred.<br>
    +          avx512_support=$(grep avx512f /proc/cpuinfo 2>&1
    > /dev/null; echo $?)<br>
               echo "avx512_support=$avx512_support" >>
    $GITHUB_OUTPUT<br>
             shell: bash<br>
           - name: setup Linux<br>
    <blockquote type="cite" cite="mid:ZiDYylFah6xu6LXk@root">
      <pre class="moz-quote-pre" wrap="">
Ping.

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+        # 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)
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">Why not just echo the "$?"?
</pre>
        </blockquote>
      </blockquote>
    </blockquote>
    removed all "tricks" and left "echo $?" as requested<br>
    <blockquote type="cite" cite="mid:ZiDYylFah6xu6LXk@root">
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">because comment above. `grep` has three possible values.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
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.

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
<snipped>

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+        echo "avx512_support=$avx512_support" >> $GITHUB_OUTPUT
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">Minor: It will be nice to echo some message about the value of this
variable. Hence, we can check this from the GitHub logs.
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
What for? Job will be skipped if there is no AVX512 support.

What is the value of the message?
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
See the rationale above.

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">

</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
<snipped>

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+        if: needs.avx512_script.outputs.avx512_support == 0
+        run: >
+          cmake -S . -B ${{ env.BUILDDIR }}
+          -G Ninja
+          -DCMAKE_BUILD_TYPE=RelWithDebInfo
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">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.
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">There is no reason for this.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
So, let's check the Debug build only. And with enabled assertions.
They are useful for testing.</pre>
    </blockquote>
    <p>replaced with Debug <br>
    </p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:ZiDYylFah6xu6LXk@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+          -DCMAKE_C_FLAGS=-march=skylake-avx512
+          -DCMAKE_C_COMPILER=gcc
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">Do we need to specify compiler manually?</pre>
        </blockquote>
      </blockquote>
    </blockquote>
    removed CMAKE_C_COMPILER at all<br>
    <blockquote type="cite" cite="mid:ZiDYylFah6xu6LXk@root">
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">Yes, CFLAG is gcc-specific.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
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?

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+      - 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
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">Should we also to set such variable in the CMakeLists.txt when we met
the corresponding condition?
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">Yes, double check.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
I meant that part of the third commit should be done within this patch,
as you've already fixed.</pre>
    </blockquote>
    Moved.<br>
    <blockquote type="cite" cite="mid:ZiDYylFah6xu6LXk@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">    local x = ffi.new("uenum_t", -10)
    local y = tobit(x)
    local z = band(x)
-- 
2.34.1
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">[1]:<a class="moz-txt-link-freetext" href="https://docs.github.com/en/actions/learn-github-actions/contexts#example-usage-of-the-steps-context">https://docs.github.com/en/actions/learn-github-actions/contexts#example-usage-of-the-steps-context</a>
[2]:<a class="moz-txt-link-freetext" href="https://www.laruence.com/x86/VCVTTSD2USI.html">https://www.laruence.com/x86/VCVTTSD2USI.html</a>

</pre>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
</pre>
    </blockquote>
  </body>
  <lt-container></lt-container>
</html>