Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: test with multiple Nim versions #133

Merged
merged 1 commit into from
Feb 8, 2022
Merged

Conversation

stefantalpalaru
Copy link
Contributor

No description provided.

@stefantalpalaru
Copy link
Contributor Author

Those Linux CI failures are due to supranational/blst#94

There are workarounds in place, but they somehow fail for Nim-1.6, which includes headers it should not include in its C output:

diff -ur nimcache_1.4/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c nimcache_1.6/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c
--- nimcache_1.4/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c 2022-01-03 03:16:29.296758105 +0100
+++ nimcache_1.6/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c 2022-01-03 03:17:13.474760284 +0100
@@ -1,16 +1,15 @@
-/* Generated by Nim Compiler v1.4.9 */
-/*   (c) 2021 Andreas Rumpf */
-/* The generated code is subject to the original license. */
+/* Generated by Nim Compiler v1.6.3 */
 /* Compiled for: Linux, amd64, gcc */
 /* Command for C compiler:
    gcc -c  -w -fmax-errors=3   -I/mnt/sde1/storage/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib -I/mnt/sde1/storage/nimbus-eth2/vendor/nim-bl
scurve/tests -o /home/stefan/.cache/nim/eip2333_key_derivation_d/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c.o /home/stefan/.cache/nim/eip2333_key_d
erivation_d/@m..@sblscurve@seth2_keygen@seth2_keygen.nim.c */
 #define NIM_INTBITS 64
 #define NIM_EmulateOverflowChecks
 
-/* section: NIM_merge_HEADERS */
-
 #include "nimbase.h"
 #include <string.h>
+#include "/mnt/sde1/storage/nimbus-eth2/vendor/nim-blscurve/blscurve/blst/../../vendor/blst/src/sha256.h"
+#include <math.h>
+#include "/mnt/sde1/storage/nimbus-eth2/vendor/nim-blscurve/blscurve/eth2_keygen/../../vendor/blst/src/vect.h"
 #include "/mnt/sde1/storage/nimbus-eth2/vendor/nim-blscurve/blscurve/blst/../../vendor/blst/bindings/blst.h"
 #undef LANGUAGE_C
 #undef MIPSEB

@stefantalpalaru
Copy link
Contributor Author

This part of the recursive diff might be the key to our problem:

Only in nimcache_1.4: @m..@sblscurve@sblst@ssha256_abi.nim.c
Only in nimcache_1.4: @m..@sblscurve@sblst@ssha256_abi.nim.c.o

So Nim-1.4 generates a separate C file for that Nim module, while Nim-1.6 inlines it.

@Araq
Copy link

Araq commented Jan 3, 2022

That's mysterious, Nim does not "inline" Nim modules.

@mratsim
Copy link
Contributor

mratsim commented Jan 3, 2022

I'll confirm later but I think everything that uses specific SIMD in separate Nim files (Arraymancer for instance) is also broken on 1.6

@mratsim
Copy link
Contributor

mratsim commented Jan 3, 2022

@stefantalpalaru
Copy link
Contributor Author

$ diff -ur nimcache_1.4 nimcache_1.6 | grep "^Only.*\.c$" | sort
Only in nimcache_1.4: @m..@sblscurve@sblst@ssha256_abi.nim.c
Only in nimcache_1.4: @m..@sblscurve@[email protected]
Only in nimcache_1.4: @m..@s..@snimcrypto@[email protected]
Only in nimcache_1.4: @m..@s..@snimcrypto@[email protected]
Only in nimcache_1.4: @m..@s..@snim-stint@[email protected]
Only in nimcache_1.4: @m..@s..@snim-stint@sstint@sprivate@suint_div.nim.c
Only in nimcache_1.4: stdlib_algorithm.nim.c
Only in nimcache_1.4: stdlib_sets.nim.c
Only in nimcache_1.6: @m..@sblscurve@sbls_batch_verifier.nim.c
Only in nimcache_1.6: @m..@s..@[email protected]
Only in nimcache_1.6: stdlib_digitsutils.nim.c
Only in nimcache_1.6: stdlib_dollars.nim.c

@SteadBytes
Copy link

@mratsim it looks like you're right about this hitting SIMD in separate files. Compiling Arraymancer with Nim 1.4.8 generates separate files for matrix multiplcation, whereas Nim 1.6.0 and above does not:

Only in nimcache_1.4.8: @m..@ssrc@sarraymancer@slaser@sprimitives@smatrix_multiplication@sgemm_ukernel_avx512.nim.c
Only in nimcache_1.4.8: @m..@ssrc@sarraymancer@slaser@sprimitives@smatrix_multiplication@sgemm_ukernel_generic.nim.c
Only in nimcache_1.4.8: @m..@ssrc@sarraymancer@slaser@sprimitives@smatrix_multiplication@sgemm_ukernel_sse2.nim.c

@Vindaar
Copy link

Vindaar commented Jan 8, 2022

I just did a git bisect on an arraymancer example that breaks with the AVX512 related error. The following is the PR that introduced the regression:

nim-lang/Nim#17311

edit: it even has a "fix arraymancer regression" todo...

@Vindaar Vindaar mentioned this pull request Jan 8, 2022
4 tasks
@stefantalpalaru
Copy link
Contributor Author

Could be this change in logic where types from an imported module are moved to the parent module: https://github.com/nim-lang/Nim/pull/17311/files#diff-1208ab8dfbcc04a982aeb83a28655324f8880fa9b756b83db873163dfe1fec6cL1425

@ringabout
Copy link

Related: nim-lang/Nim#19363

@stefantalpalaru stefantalpalaru merged commit 60103b8 into master Feb 8, 2022
@stefantalpalaru stefantalpalaru deleted the ci_multi_nim branch February 8, 2022 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants