-
Notifications
You must be signed in to change notification settings - Fork 58
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
chafa 1.14.1 fails to build on i686 #203
Comments
A similar error occurs on Snow Leopard i386 buildbot in MacPorts:
This is most likely due to this code fragment in configure.ac. For optimization purposes it should be called strictly for x86-64 compatible platforms, but for some reason this check is not done. |
Thanks for reporting this. I think the issue in both cases is that the The AVX code is guarded by a run-time check, so it won't be used on CPUs that don't support AVX. |
Would you be able to quickly test a fix? I don't have a way to reproduce the build error at the moment, but I have this in mind: diff --git a/chafa/internal/chafa-avx2.c b/chafa/internal/chafa-avx2.c
index ae20830..8efbc9d 100644
--- a/chafa/internal/chafa-avx2.c
+++ b/chafa/internal/chafa-avx2.c
@@ -21,6 +21,7 @@
#include <emmintrin.h>
#include <immintrin.h>
+#include <smmintrin.h> /* For _mm_extract_epi64 */
#include "chafa.h"
#include "internal/chafa-private.h"
|
With that patch nothing changes: chafa__do_build.log Here's the configure log for completeness: chafa__do_configure.log |
Thanks. The logs indicate that |
This built with diff --git a/chafa/internal/chafa-avx2.c b/chafa/internal/chafa-avx2.c
index ae20830..7f11687 100644
--- a/chafa/internal/chafa-avx2.c
+++ b/chafa/internal/chafa-avx2.c
@@ -21,9 +21,21 @@
#include <emmintrin.h>
#include <immintrin.h>
+#include <smmintrin.h>
#include "chafa.h"
#include "internal/chafa-private.h"
+/* _mm_extract_epi64() (pextrq) is not available in 32-bit mode. Work around
+ * it. This needs to be a macro, as the compiler expects an integer constant
+ * for n. */
+#if defined __x86_64__ && !defined __ILP32__
+# define extract_128_epi64(i, n) _mm_extract_epi64 ((i), (n))
+#else
+# define extract_128_epi64(i, n) \
+ ((((guint64) _mm_extract_epi32 ((i), (n) * 2 + 1)) << 32) \
+ | _mm_extract_epi32 ((i), (n) * 2))
+#endif
+
gint
calc_error_avx2 (const ChafaPixel *pixels, const ChafaColorPair *color_pair,
const guint32 *sym_mask_u32)
@@ -96,14 +108,14 @@ calc_colors_avx2 (const ChafaPixel *pixels, ChafaColorAccum *accums_out,
accum_bg_128 = _mm_add_epi16 (_mm256_extracti128_si256 (accum_bg, 0),
_mm256_extracti128_si256 (accum_bg, 1));
((guint64 *) accums_out) [0] =
- (guint64) _mm_extract_epi64 (accum_bg_128, 0)
- + (guint64) _mm_extract_epi64 (accum_bg_128, 1);
+ extract_128_epi64 (accum_bg_128, 0)
+ + extract_128_epi64 (accum_bg_128, 1);
accum_fg_128 = _mm_add_epi16 (_mm256_extracti128_si256 (accum_fg, 0),
_mm256_extracti128_si256 (accum_fg, 1));
((guint64 *) accums_out) [1] =
- (guint64) _mm_extract_epi64 (accum_fg_128, 0)
- + (guint64) _mm_extract_epi64 (accum_fg_128, 1);
+ extract_128_epi64 (accum_fg_128, 0)
+ + extract_128_epi64 (accum_fg_128, 1);
}
/* 32768 divided by index. Divide by zero is defined as zero. */
@@ -143,5 +155,5 @@ chafa_color_accum_div_scalar_avx2 (ChafaColorAccum *accum, guint16 divisor)
accum_128 = _mm_loadl_epi64 ((const __m128i *) accum);
divisor_128 = _mm_set1_epi16 (invdiv16 [divisor]);
accum_128 = _mm_mulhrs_epi16 (accum_128, divisor_128);
- *((guint64 *) accum) = _mm_extract_epi64 (accum_128, 0);
+ *((guint64 *) accum) = extract_128_epi64 (accum_128, 0); |
Yep, Void CI passes with this patch: https://github.com/void-linux/void-packages/actions/runs/9587902854/job/26438847164?pr=50873 Thank you for your time! |
Great! Thanks for distributing it. Are you ok carrying the patch for a couple of weeks, until I can make a new release? |
Sure, no problem :) |
Build log on Void Linux i686 CI: https://github.com/void-linux/void-packages/actions/runs/9554806547/job/26336662670?pr=50873
The text was updated successfully, but these errors were encountered: