<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi, Sergey<br>
</p>
<div class="moz-cite-prefix">On 12.04.2024 13:27, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On 11.04.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">From: Sergey Bronnikov <a class="moz-txt-link-rfc2396E" href="mailto:sergeyb@tarantool.org"><sergeyb@tarantool.org></a>
This commit adds a workflow for building and testing with enabled
AVX512.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/with enabled AVX512/with AVX512 enabled/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Needed for tarantool/tarantool#9595
Related to tarantool/tarantool#6787
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/Related/Relates/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<pre class="moz-quote-pre" wrap="">
| git log | grep Relates | wc -l
| 29
| git log | grep Related | wc -l
| 7
</pre>
</blockquote>
<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">---
.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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I suppose it shouldn't be a part of setup-linux action,</pre>
</blockquote>
<p>It was requested by Max.<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<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 type="cite">
<pre class="moz-quote-pre" wrap="">+ id: avx512_script
+ run: |
+ # Set avx512_support environment variable to 'true' when AVX512
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/avx512_support/the avx512_support/
</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ # is supported and 'false' otherwise.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<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 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>
because comment above. `grep` has three possible values.<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
Changed to AVX512F.<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<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>
<p>What for? Job will be skipped if there is no AVX512 support.</p>
<p><span class="HwtZe" lang="en"><span class="jCAhz ChMk0b"><span
class="ryNqvb">What is the value of the message?</span></span></span><span
class="ZSCsVd"></span></p>
<div class="OvtS8d"><br>
</div>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<pre class="moz-quote-pre" wrap="">
Side note: Do I understand correctly that GH-actions output is always a
string that evaluates to `true` regardless of its content?</pre>
</blockquote>
0/1<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+jobs:
+ test-avx512:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: I suggest adding a name for the job to be consistent with other
jobs. Something like "LuaJIT avx512""</pre>
</blockquote>
Added.<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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
</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>
There is no reason for this.<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<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>
Yes, CFLAG is gcc-specific.<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<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>
Yes, double check.<br>
<blockquote type="cite" cite="mid:ZhkMnDv9fNhiyHCx@root">
<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>
</body>
</html>