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
Fails to build with x32 abi #74
Comments
This is the commit where it is introduced |
It looks like the x86_64 inline asm is enabled on x32. I don't have any x32 system to test on, but I guess a fix would be to change the line: #if defined(__x86_64__) && (defined(__AVX__) || !defined(__GNUC__))
to: #if defined(__x86_64__) && (defined(__AVX__) || defined(__ILP32__) || !defined(__GNUC__))
That way, a suitable implementation with intrinsics should be used and the inline asm one not reached by the conditional compiles. Slightly better performance could be possible through addition of x32 specific inline asm. I think it can be something like (untested, and diff is against upstream yescrypt): --- yescrypt-opt.c 2018-07-10 17:51:52 +0000
+++ yescrypt-opt-x32.c 2018-12-17 19:29:36 +0000
@@ -515,6 +515,11 @@
#undef MAYBE_MEMORY_BARRIER
#define MAYBE_MEMORY_BARRIER \
__asm__("" : : : "memory");
+#ifdef __ILP32__ /* x32 */
+#define REGISTER_PREFIX "e"
+#else
+#define REGISTER_PREFIX "r"
+#endif
#define PWXFORM_SIMD(X) { \
__m128i H; \
__asm__( \
@@ -524,8 +529,8 @@
"pmuludq %1, %0\n\t" \
"movl %%eax, %%ecx\n\t" \
"shrq $0x20, %%rax\n\t" \
- "paddq (%3,%%rcx), %0\n\t" \
- "pxor (%4,%%rax), %0\n\t" \
+ "paddq (%3,%%" REGISTER_PREFIX "cx), %0\n\t" \
+ "pxor (%4,%%" REGISTER_PREFIX "ax), %0\n\t" \
: "+x" (X), "=x" (H) \
: "d" (Smask2), "S" (S0), "D" (S1) \
: "cc", "ax", "cx"); \ Please test. Yet another kind of fix would be to define two limited scope variables of type |
let me try second solution out |
@kraj @solardiz That's odd… i686 builds [1] on Fedora don't show this issue. [1] https://kojipkgs.fedoraproject.org//packages/libxcrypt/4.4.1/1.fc30/data/logs/i686/build.log |
x32 is not something you can try on Fedora. |
@solardiz the inline asm change is building successfully, waiting for runtime results. hopefully tomorrow |
@kraj So what are the runtime results? And what project is this for? Do you actually use yescrypt there or is it merely getting compiled along with the rest of libxcrypt, then left unused? I'd like to know whether to merge this fix upstream. Thanks! |
@kraj It would be nice, if you would open a PR with this patch here as well. |
Here is regression seen on x32 builds
The text was updated successfully, but these errors were encountered: