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

update to work around numpy issue 24067 #101

Merged
merged 7 commits into from Jul 16, 2023
Merged

Conversation

mattip
Copy link
Collaborator

@mattip mattip commented Jun 30, 2023

No description provided.

@mattip
Copy link
Collaborator Author

mattip commented Jul 2, 2023

something is off with the cross-compilation for arm64. Is this a known problem?

@mattip
Copy link
Collaborator Author

mattip commented Jul 11, 2023

The macos arm64 cross build is failing to compile.

Maybe we are using an older gfortran-cross that does not know about SVE, there are errors about intrinsics

../kernel/arm64/sgemm_kernel_sve_v2x8.S:873:2: error: instruction requires: sve

@isuruf, we are using this one, any thoughts?

curl -L -O https://github.com/isuruf/gcc/releases/download/gcc-11.3.0-2/gfortran-darwin-arm64-cross.tar.gz

@martin-frbg: is there an environment variable that would skip the SVE intrinsics?

@martin-frbg
Copy link

martin-frbg commented Jul 11, 2023

NO_SVE might work but this looks more like the (cross)compiler is expecting a "+sve" on the -march= option and not getting it. (does VORTEX even support SVE? not sure how it even gets there... will try to find time to read your log later)

@martin-frbg
Copy link

That build is using clang to cross-compile to arm64 under OSX ? Pretty sure that is a cursed configuration - never tried, never could have worked. The only OSX ARM64 target is VORTEX (M1/M2 family) which does not support SVE, and none of the usual DYNAMIC_ARCH targets for ARM64 make sense in this configuration to my knowledge. (DYNAMIC_ARCH even is anything but VORTEX support).
But the primary issue is simply that the -march= option setting in Makefile.arm64 is still geared towards gcc, and while there was some attempt made to handle "or CLANG" it seems I did not add this on all nested conditionals so some version checks may still be failing. This was probably aggravated by the unconditional addition of SVE kernels for the Graviton cpus, so any fallback to non-SVE -march settings for older "gcc" (that may be not gcc at all) would now result in build breakage.
In short, if M1/M2 OSX is your only goal, TARGET=VORTEX without DYNAMIC_ARCH should work. If you really want DYNAMIC_ARCH, use gcc for now (and it would probably make a lot more sense to set TARGET=ARMV8, as that is supposed to be the least common denominator of all cpus in DYNAMIC_ARCH, used to select the compiler options when compiling all the common code in interface,driver,lapack,etc)

@martin-frbg
Copy link

Probably fixed by OpenMathLib/OpenBLAS#4136 if you want to try cherry-picking that one.

@mattip
Copy link
Collaborator Author

mattip commented Jul 11, 2023

I tried the single commit 16624159 using the patch_source command. It does not seem to change the error.

@martin-frbg
Copy link

Strange, what is your version of clang ? (Compiles fine for me locally on Samsung Galaxy Tab, need to setup something similar to your CI job to reproduce the circumstances of your crossbuild but cannot think of any fundamental difference)
Pity that your build log only shows the warnings, not the invocation lines to check if any cpu-specific -march gets added at all.

@mattip
Copy link
Collaborator Author

mattip commented Jul 12, 2023

Pity that your build log only shows the warnings, not the invocation lines

I remember turning on set +e caused some problems on macos, but maybe that has been improved. Let's see...

@martin-frbg
Copy link

Looks like my test job (Xcode 12.4 on Azure CI) autoselected NO_SVE in c_check and consequently left out the Neoverse targets.

@mattip
Copy link
Collaborator Author

mattip commented Jul 12, 2023

ahh, the patch_source function was outdated and exited early

@mattip
Copy link
Collaborator Author

mattip commented Jul 12, 2023

Pity that your build log only shows the warnings, not the invocation lines

Is there some flag I could add to make to show the invocations? I am using

CFLAGS=' -arch arm64 -fvisibility=protected'
make BUFFERSIZE=20 DYNAMIC_ARCH=1 USE_OPENMP=0 NUM_THREADS=64 BINARY=64 INTERFACE64=1 SYMBOLSUFFIX=64_ OBJCONV=/Users/runner/work/openblas-libs/openblas-libs/objconv/objconv TARGET=VORTEX

and

CFLAGS=' -arch arm64 -fvisibility=protected'
make BUFFERSIZE=20 DYNAMIC_ARCH=1 USE_OPENMP=0 NUM_THREADS=64 BINARY=64 TARGET=VORTEX

@mattip
Copy link
Collaborator Author

mattip commented Jul 12, 2023

Clang version is Apple clang version 12.0.5 (clang-1205.0.22.11)

@mattip
Copy link
Collaborator Author

mattip commented Jul 12, 2023

There is also this

clang: error: the clang compiler does not support '-mtune=neoverse-v1'

@martin-frbg
Copy link

Thanks - have updated my CI job to the same compiler version and substituted "cortex-x1" for neoverse-v1 with clang after checking with --print-supported-cpus. This is probably not the final fix though - according to spack/spack#36481 (comment) , newer AppleClang appears to accept neoverse-v1 just fine but has done away with "vortex" in favor of "apple-m1"

@mattip
Copy link
Collaborator Author

mattip commented Jul 13, 2023

This is probably not the final fix though - according to spack/spack#36481 (comment) , newer AppleClang appears to accept neoverse-v1 just fine but has done away with "vortex" in favor of "apple-m1"

Bleh.

@martin-frbg
Copy link

my test build succeeds now after adding a bunch of silly casts to various sve kernels in #4140 - just to help clang around a perceived ambiguity

@mattip
Copy link
Collaborator Author

mattip commented Jul 13, 2023

Cool, it is green. Do I understand correctly that you are not downloading a cross compiler, rather using the one in XCode?

@martin-frbg
Copy link

Yes (or at least I think so :/ )

@mattip
Copy link
Collaborator Author

mattip commented Jul 14, 2023

It seems that your cross-compile build does not use a fortran compiler. The cross-compile build here does try very hard to get a fortran compiler. Is the work here to get a gfortran compiler unecessary?

@martin-frbg
Copy link

Not unnecessary if you want a full-featured and up to date LAPACK - at least not in general, only for the very specific issue of getting the sve kernels compiled. Anyone without a Fortran compiler gets an f2c-mangled dose of old LAPACK roughly corresponding to 3.9.0 with backported fixes - the newer additions mostly use too many modern Fortran features to be easily translatable, so any 3.10/3.11 functions in C are likely to be incomplete or dummies.

@mattip mattip changed the title update to commit 2183dbcf to work around numpy issue 24067 update to work around numpy issue 24067 Jul 14, 2023
@martin-frbg
Copy link

looks like something in your unix build machinery does not like the now-empty patch_source function

@mattip
Copy link
Collaborator Author

mattip commented Jul 16, 2023

Build is green, lets go with this. Then NumPy and Scipy can change to use "3d31191b" rather than "v0.3.23", to work around the numpy/numpy#24067

@mattip mattip merged commit a64e515 into MacPython:main Jul 16, 2023
18 of 19 checks passed
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.

None yet

2 participants