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

Draft on 32-bit CI #1564

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Draft on 32-bit CI #1564

merged 2 commits into from
Nov 2, 2023

Conversation

albinahlback
Copy link
Collaborator

No description provided.

@fredrik-johansson
Copy link
Collaborator

Hope this works!

I think it would be prudent to enable asserts for the 32-bit tests.

@albinahlback
Copy link
Collaborator Author

Hope this works!

I think it would be prudent to enable asserts for the 32-bit tests.

Sounds like a good idea!

@albinahlback
Copy link
Collaborator Author

Doesn't seem to pass the tests...

@edgarcosta
Copy link
Member

@tornaria do tests pass on your 32bit machine?

@albinahlback
Copy link
Collaborator Author

Looks like fmpz_set_str is the problem.

@albinahlback
Copy link
Collaborator Author

Could it be that DIGITS_PER_LIMB should be 10 for 32-bit fmpz_set_str, @fredrik-johansson?

@tornaria
Copy link
Contributor

tornaria commented Nov 1, 2023

@tornaria do tests pass on your 32bit machine?

Everything passes here:

https://github.com/void-linux/void-packages/actions/runs/6700948977/job/18207675113?pr=46832

I'm using released 3.0 + #1545 + #1546.

@tornaria
Copy link
Contributor

tornaria commented Nov 1, 2023

Also, btw, the job I posted builds and checks eclib, singular, and sagemath with the new flintlib.

@fredrik-johansson
Copy link
Collaborator

Could it be that DIGITS_PER_LIMB should be 10 for 32-bit fmpz_set_str, @fredrik-johansson?

It should be 9, but

    tmp = TMP_ALLOC(sizeof(mp_limb_t) * (slen / DIGITS_PER_LIMB) + 2);

looks like it should be

    tmp = TMP_ALLOC(sizeof(mp_limb_t) * (slen / DIGITS_PER_LIMB + 2));

@tornaria
Copy link
Contributor

tornaria commented Nov 1, 2023

I'm now testing 4b27c91 on i686 (glibc). However, if this is an issue with allocation maybe it's triggered in musl libc and not in glibc.

@fredrik-johansson
Copy link
Collaborator

It's possible that the bug only manifests itself with --enable-assert which does change the behavior of TMP_ALLOC.

@tornaria
Copy link
Contributor

tornaria commented Nov 1, 2023

Could also be a bug in gcc since you are getting 13.2 and I'm running 12.2. Is it feasible to test with an earlier version of alpine?

@tornaria
Copy link
Contributor

tornaria commented Nov 1, 2023

Build and check of 4b27c91 finished without any failure.

@fredrik-johansson
Copy link
Collaborator

Trying this on a 32-bit debian in VirtualBox. The fmpz_is_prime test passes, but valgrind does show a bunch of errors. They go away when fixing the TMP_ALLOC line mentioned above.

@tornaria
Copy link
Contributor

tornaria commented Nov 1, 2023

It's possible that the bug only manifests itself with --enable-assert which does change the behavior of TMP_ALLOC.

I redid the build + check adding --enable-assert to configure, and the test works fine.

@tornaria
Copy link
Contributor

tornaria commented Nov 1, 2023

Trying this on a 32-bit debian in VirtualBox. The fmpz_is_prime test passes, but valgrind does show a bunch of errors. They go away when fixing the TMP_ALLOC line mentioned above.

IME, musl allocator is less forgiving to allocation mistakes, so it's good that you test on alpine.

I do test musl, but only x86_64 since void doesn't do i686-musl binaries (I'd have to rebuild the whole system).

While running the whole testsuite on --enable-assert I got:

ecm....t-ecm: src/ulong_extras/mulmod_preinv.c:22: n_mulmod_preinv: Assertion `a < n' failed.

@fredrik-johansson
Copy link
Collaborator

The allocation bug is fixed in 6c92f36.

@albinahlback
Copy link
Collaborator Author

I will increase the test multiplier once it passes all the tests so that it matches in time with Ubuntu GCC AVX2 x10, so that we may detect more bugs.

@albinahlback
Copy link
Collaborator Author

Test fails for fmpz_factor_ecm

@fredrik-johansson
Copy link
Collaborator

The ECM bug may be fixed in 016de13. Tests now pass for me on 32-bit.

Note to self: the ECM code needs a thorough review.

@albinahlback albinahlback marked this pull request as ready for review November 2, 2023 10:58
@albinahlback albinahlback merged commit 10a5121 into flintlib:trunk Nov 2, 2023
12 checks passed
@fredrik-johansson
Copy link
Collaborator

Nice!

@albinahlback albinahlback mentioned this pull request Nov 2, 2023
@albinahlback albinahlback deleted the 32bit branch November 2, 2023 11:28
@tornaria
Copy link
Contributor

tornaria commented Nov 2, 2023

FWIW, I built and checked 8528d83 on i686 with --enable-assert. Everything passes now.

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.

4 participants