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