Make use of the full range of AUX matrices #33
Conversation
gdonval
requested review from
SylvainCorlay,
dalcinl,
davidbrochart,
minrk and
traversaro
as code owners
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
|
@conda-forge-admin, please rerender |
|
Those osx failures are unrelated to my code (different file, different folder, error completely unrelated to anything I have done). |
|
Alright, I took the easy approach to solving the osx problems: ignore those specific warnings.
In short, this is ready for release. |
|
Any news/updates on this? Or is this specific feedstock abandoned? |
|
Well. still waiting for review... and since I'm not added as a maintainer yet, nothing I can do. |
|
@gdonval I'm not in a position to review this PR. But if you ask me explicitly to merge and you take accountability for the outcome, I'm very happy to click the button. |
|
@conda-forge-admin rerender |
|
@dalcinl: I'll recheck tomorrow to make sure all looks good and I'll ping you if so. Thanks a lot! |
In the future, please add a mention to the mantainer team (i.e. @conda-forge/scalapack) when you write that a PR is ready for review. Otherwise, it is quite easy for mantainers to miss a message. |
recipe/build.sh
Outdated
| # Workaround for implicit function declarations on osx | ||
| export CFLAGS="$CFLAGS -std=gnu99 -Wno-implicit-function-declaration" | ||
| export CXXFLAGS="$CXXFLAGS -std=gnu99 -Wno-implicit-function-declaration" | ||
| export FFLAGS="${FFLAGS} -fallow-argument-mismatch" | ||
| export OMPI_FCFLAGS=${FFLAGS} |
There was a problem hiding this comment.
Can you explain why this is necessary?
recipe/build.sh
Outdated
| @@ -8,7 +17,7 @@ if [[ "$target_platform" == "osx-64" ]]; then | |||
| fi | |||
| fi | |||
|
|
|||
| if [[ "$target_platform" == linux-* || "$target_platform" == "osx-arm64" ]] | |||
There was a problem hiding this comment.
I guess the workaround is not needed anymore in osx-arm64 due to an update in the compiler, at this point probably we can also remove the comment in the if referring to gfortran 9?
|
@gdonval did you get a chance to check things? Just a ping since this will improve a few downstream things. |
|
@gdonval @Chronum94 are you still interested in this? @Chronum94 even if you were not the original author of the PR, if the original author does not reply you can open a new PR with your own improvements/addressing the reviews. |
The gist of it is that we have this signature `mr2d_malloc(Int n)`, bounding the internal `malloc` to `Int`, which is Scalapack's indexing type. This is not how `malloc` is defined and means that maximum AUX matrix size is artifically limited on 64-bit systems to 2GB. This PR operates this transformation: `mr2d_malloc(Int n) -> mr2d_malloc(size_t n)`, yet ensures `Int -> size_t` conversion does not involve Int-negative values and ensures that no 64-bit values are passed to `malloc` on 32-bit systems. The main advantage of this on 64-bit system allows the use of the **full** _signed_ 32-bit indexing range instead of `range / element size`. E.g. the max AUX matrix size is now 16GB instead of 2GB previously. With this, "standard" 32-bit Scalapack and Blas/Lapack can still be used in programs like GPAW. Details (and full commit history) are in Reference-ScaLAPACK/scalapack#85 which does not seem to receive much attention. This patch is a way to provide that feature to conda users in the meantime.
…nda-forge-pinning 2024.10.31.23.25.38
|
Rebased to just the patch commit, since this no longer needs any build.sh patches. Thanks @gdonval! |
|
Sorry for the delay. Happy this got merged! 🎉 |
The gist of it is that we have this signature
mr2d_malloc(Int n), bounding the internalmalloctoInt, which is Scalapack's indexing type. This is not howmallocis defined and means that maximum AUX matrix size is artifically limited on 64-bit systems to 2GB.This PR operates this transformation:
mr2d_malloc(Int n) -> mr2d_malloc(size_t n), yet ensuresInt -> size_tconversion does not involve Int-negative values and ensures that no 64-bit values are passed tomallocon 32-bit systems.The main advantage of this on 64-bit system allows the use of the full signed 32-bit indexing range instead of
range / element size. E.g. the max AUX matrix size is now 16GB instead of 2GB previously.With this, "standard" 32-bit Scalapack and Blas/Lapack can still be used in programs like GPAW.
Details (and full commit history) are in
Reference-ScaLAPACK/scalapack#85 which does not seem to receive much attention.
This patch is a way to provide that feature to conda users in the meantime.
Checklist
Reset the build number to0(if the version changed)conda-smithy(Use the phrase@conda-forge-admin, please rerenderin a comment in this PR for automated rerendering)