search

openvenues / libpostal

A C library for parsing/normalizing street addresses around the world. Powered by statistical NLP and open geo data.
MIT License
4.08k stars 421 forks source link

fixes for SSE/memalign issue in crf_context test #632

Closed albarrentine closed 1 year ago

albarrentine commented 1 year ago

from #597 #592 and #578 there's an issue in the memory alignment boundary when running the tests with SSE and the remez algorithm which is looping over 4 doubles (it uses 16 bytes in a few places for defining some constants but should actually be guaranteed to be aligned to 32 bytes) and the crf_context test calls for 3x3 labels which can create undefined behavior. It strangely hasn't shown up previously and shouldn't in normal parser usage but that's only because the parser happens to have L=20 labels and the CRF's context matrix is L x L x T, so for any number of items T, it's always aligning something proportional to 4.

Trying this piece out first with the new Github Actions build, then will add a standards-compliant implementation of an aligned realloc.

missinglink commented 1 year ago

Thanks @albarrentine, this fixes linux x86 but unfortunately introduced an error on Mac ARM (M2 Chipset):

sysctl -a | grep machdep.cpu
machdep.cpu.cores_per_package: 8
machdep.cpu.core_count: 8
machdep.cpu.logical_per_package: 8
machdep.cpu.thread_count: 8
machdep.cpu.brand_string: Apple M2
make -j8
/Library/Developer/CommandLineTools/usr/bin/make  all-recursive
Making all in src
gcc -DHAVE_CONFIG_H -I.. -I/usr/local/include    -Wall -Wextra -Wno-unused-function -Wformat -Werror=format-security -Winit-self -Wno-sign-compare -DLIBPOSTAL_DATA_DIR='"/data/libpostal"' -g -mfpmath=sse -msse2 -DUSE_SSE -g -O2 -O3  -MT libpostal-strndup.o -MD -MP -MF .deps/libpostal-strndup.Tpo -c -o libpostal-strndup.o `test -f 'strndup.c' || echo './'`strndup.c
gcc -DHAVE_CONFIG_H -I.. -I/usr/local/include    -Wall -Wextra -Wno-unused-function -Wformat -Werror=format-security -Winit-self -Wno-sign-compare -DLIBPOSTAL_DATA_DIR='"/data/libpostal"' -g -mfpmath=sse -msse2 -DUSE_SSE -g -O2 -O3  -MT libpostal-main.o -MD -MP -MF .deps/libpostal-main.Tpo -c -o libpostal-main.o `test -f 'main.c' || echo './'`main.c
gcc -DHAVE_CONFIG_H -I.. -I/usr/local/include    -Wall -Wextra -Wno-unused-function -Wformat -Werror=format-security -Winit-self -Wno-sign-compare -DLIBPOSTAL_DATA_DIR='"/data/libpostal"' -g -mfpmath=sse -msse2 -DUSE_SSE -g -O2 -O3  -MT libpostal-json_encode.o -MD -MP -MF .deps/libpostal-json_encode.Tpo -c -o libpostal-json_encode.o `test -f 'json_encode.c' || echo './'`json_encode.c
gcc -DHAVE_CONFIG_H -I.. -I/usr/local/include    -Wall -Wextra -Wno-unused-function -Wformat -Werror=format-security -Winit-self -Wno-sign-compare -DLIBPOSTAL_DATA_DIR='"/data/libpostal"' -g -mfpmath=sse -msse2 -DUSE_SSE -g -O2 -O3  -MT libpostal-file_utils.o -MD -MP -MF .deps/libpostal-file_utils.Tpo -c -o libpostal-file_utils.o `test -f 'file_utils.c' || echo './'`file_utils.c
gcc -DHAVE_CONFIG_H -I.. -I/usr/local/include    -Wall -Wextra -Wno-unused-function -Wformat -Werror=format-security -Winit-self -Wno-sign-compare -DLIBPOSTAL_DATA_DIR='"/data/libpostal"' -g -mfpmath=sse -msse2 -DUSE_SSE -g -O2 -O3  -MT libpostal-string_utils.o -MD -MP -MF .deps/libpostal-string_utils.Tpo -c -o libpostal-string_utils.o `test -f 'string_utils.c' || echo './'`string_utils.c
gcc -DHAVE_CONFIG_H -I.. -I/usr/local/include    -Wall -Wextra -Wno-unused-function -Wformat -Werror=format-security -Winit-self -Wno-sign-compare -DLIBPOSTAL_DATA_DIR='"/data/libpostal"' -g -mfpmath=sse -msse2 -DUSE_SSE -g -O2 -O3  -MT utf8proc/libpostal-utf8proc.o -MD -MP -MF utf8proc/.deps/libpostal-utf8proc.Tpo -c -o utf8proc/libpostal-utf8proc.o `test -f 'utf8proc/utf8proc.c' || echo './'`utf8proc/utf8proc.c
/bin/sh ../libtool  --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I.. -I/usr/local/include    -Wall -Wextra -Wno-unused-function -Wformat -Werror=format-security -Winit-self -Wno-sign-compare -DLIBPOSTAL_DATA_DIR='"/data/libpostal"' -g -mfpmath=sse -msse2 -DUSE_SSE -g -O2 -O2 -D LIBPOSTAL_EXPORTS  -MT libpostal_la-strndup.lo -MD -MP -MF .deps/libpostal_la-strndup.Tpo -c -o libpostal_la-strndup.lo `test -f 'strndup.c' || echo './'`strndup.c
/bin/sh ../libtool  --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I.. -I/usr/local/include    -Wall -Wextra -Wno-unused-function -Wformat -Werror=format-security -Winit-self -Wno-sign-compare -DLIBPOSTAL_DATA_DIR='"/data/libpostal"' -g -mfpmath=sse -msse2 -DUSE_SSE -g -O2 -O2 -D LIBPOSTAL_EXPORTS  -MT libpostal_la-libpostal.lo -MD -MP -MF .deps/libpostal_la-libpostal.Tpo -c -o libpostal_la-libpostal.lo `test -f 'libpostal.c' || echo './'`libpostal.c
clang: warning: argument unused during compilation: '-msse2' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-msse2' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-msse2' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-msse2' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-msse2' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-msse2' [-Wunused-command-line-argument]
error: unknown FP unit 'sse'
error: unknown FP unit 'sse'
make[2]: *** [libpostal-string_utils.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [libpostal-file_utils.o] Error 1
error: unknown FP unit 'sse'
error: unknown FP unit 'sse'
make[2]: *** [libpostal-json_encode.o] Error 1
make[2]: *** [libpostal-main.o] Error 1
error: unknown FP unit 'sse'
error: unknown FP unit 'sse'
make[2]: *** [utf8proc/libpostal-utf8proc.o] Error 1
make[2]: *** [libpostal-strndup.o] Error 1
libtool: compile:  gcc -DHAVE_CONFIG_H -I.. -I/usr/local/include -Wall -Wextra -Wno-unused-function -Wformat -Werror=format-security -Winit-self -Wno-sign-compare -DLIBPOSTAL_DATA_DIR=\"/data/libpostal\" -g -mfpmath=sse -msse2 -DUSE_SSE -g -O2 -O2 -D LIBPOSTAL_EXPORTS -MT libpostal_la-strndup.lo -MD -MP -MF .deps/libpostal_la-strndup.Tpo -c strndup.c  -fno-common -DPIC -o .libs/libpostal_la-strndup.o
libtool: compile:  gcc -DHAVE_CONFIG_H -I.. -I/usr/local/include -Wall -Wextra -Wno-unused-function -Wformat -Werror=format-security -Winit-self -Wno-sign-compare -DLIBPOSTAL_DATA_DIR=\"/data/libpostal\" -g -mfpmath=sse -msse2 -DUSE_SSE -g -O2 -O2 -D LIBPOSTAL_EXPORTS -MT libpostal_la-libpostal.lo -MD -MP -MF .deps/libpostal_la-libpostal.Tpo -c libpostal.c  -fno-common -DPIC -o .libs/libpostal_la-libpostal.o
clang: warning: argument unused during compilation: '-msse2' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-msse2' [-Wunused-command-line-argument]
error: unknown FP unit 'sse'
error: unknown FP unit 'sse'
make[2]: *** [libpostal_la-strndup.lo] Error 1
make[2]: *** [libpostal_la-libpostal.lo] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2
missinglink commented 1 year ago

oh I see now the edit to the install instructions, adding --disable-sse2 allows the build to succeed on the Mac M2 chipset.

albarrentine commented 1 year ago

@missinglink right, in terms of the build for x86 vs. M1/M2, it reverts the NEON PR/autoconf checks almost entirely and uses the guidance from before, which was to disable SSE2 on ARM architectures. I think there's likely a one-line fix to get that PR working for all cases but nothing immediately jumped out from staring at the changes. Adding some more details to that PR if anyone wants to give it a shot.

ddelange commented 1 year ago

Hi :wave:

Ended up here via git blame: around this commit we started seeing a compilation error:

gcc: error: unrecognized command-line option ‘-msse2’`
gcc: error: unrecognized command-line option ‘-mfpmath=sse’

Might it be related to these changes?

albarrentine commented 1 year ago

@ddelange it looks like one of the builds is on ARM. The guidance on ARM architectures has been reverted back to what it was prior to #578 (which allowed ARM/Mac M1/2 to convert SIMD instructions to NEON), which was to disable SSE on ARM architectures with --disable-sse2 in the configure step until there's a version of #578 that works for all platforms. That's easy to do with an if step in the Github Action per platform.

Alternatively, can merge a pull request if anyone has time to get the autoconf fu figured out to make it configless (i.e. check if SSE instructions are available and disable otherwise). This looks like a possibility: https://www.gnu.org/software/autoconf-archive/ax_check_x86_features.html, can add it to the m4 directory similar to the ax_cblas.m4 and add the associated pieces in configure.ac