Expose MKL linkage, fixes segfaults with lapack crate.

[geo-ant]

Hi, I've tried using the lapack and lapack-src crates like this and ran into segmentation faults and/or runtime parameter errors from the LAPACK backend. It didn't take too long to figure out that this was caused by the intel-mkl-src crate linking the ILP64 version (long and int are 64-bit) of MKL, whereas the lapack crate exposes the LP64 interface (long is 64 bit, int is 32 bit). So using lapack and lapack-src like this

lapack = "0.20"
lapack-src = { version = "0.11", features = ["intel-mkl"] }

will produce segfaults and lapack runtime errors. So I made two changes

Changes

1. BREAKING Make the sequential LP64 version linked when the intel-mkl feature is selected, so that this works together with the lapack crate more out of the box. This is a breaking change, but I do think it's justified for the out-of-the-box compatibility with the lapack crate.
2. Expose a set of features intel-mkl-*-*-* to allow a user to select the desired linkage.

[IvanUkhov]

Hello,

Thank you for looking into this. In general, the strategy is to not try to forward any src crate's features but instead let the end user take care of it. So in your case, I would do as follows:

lapack = "0.20"
lapack-src = { version = "0.11", features = ["intel-mkl"] }
intel-mkl-src = { version = "0.8", features = ["mkl-static-lp64-seq"] }

But I agree it should ideally work out of the box. I would propose we keep only this line:

intel-mkl = ["intel-mkl-static-lp64-seq"]

And explain in the readme that if something else is needed, the user should not activate intel-mkl via this crate and instead should just depend on intel-mkl-src and select any feature they like.

Alternatively, we keep intel-mkl as it was without adding any new features, but have a nice explanation in the readme.

[geo-ant]

Hi, thanks for getting back to me. Correct me if I'm misunderstanding, but if we make the intel-mkl feature link against lp64-seq, then including another intel-mkl-src (maybe a parallel backend) would lead to duplicate symbols, right? The correct way should then be not to include any lapack backend via lapack-src? That means the user doesn't include lapack-src at all, but just the correct intel-mkl-src, right?

lapack-src = { version = "0.11", features = ["intel-mkl"] }
intel-mkl-src = { version = "0.8", features = ["mkl-static-lp64-seq"] }J

I think it's definitely okay to say the only way to link intel-mkl via lapack-src is in the lp64 parallel variant. It just has to be clearly documented. Do you want me to go ahead and change that?

Also I have no idea why the mac-os tests are failing, I don' t think I did anything to those :D

[IvanUkhov]

In my example, I was assuming intel-src would not activate any feature by default. So the user chooses that source via the feature but tunes the specific by depending on the source crate and tweaking its features directly. Yes, in this case, depending on lapack-src is not adding anything and can technically be dropped. But I would keep it still to make the intention clear. “I am choosing this source, and it will now be compiled, but let me adjust how.”

Yes, the parallel one is perhaps a better default choice. With a good description of what to do if something else is needed.

If you rebase on main, that error should be gone.

[geo-ant]

In my example, I was assuming intel-src would not activate any feature by default. So the user chooses that source via the feature but tunes the specific by depending on the source crate and tweaking its features directly. Yes, in this case, depending on lapack-src is not adding anything and can technically be dropped. But I would keep it still to make the intention clear. “I am choosing this source, and it will now be compiled, but let me adjust how.”

Yes, the parallel one is perhaps a better default choice. With a good description of what to do if something else is needed.

If you rebase on main, that error should be gone.

Hey, sorry if I'm being obtuse here. I've made a simplified proposal that I think does what you want. If not, let me know. I've now made it that intel-mkl statically links against the parallel MKL. We also expose one more feature intel-mkl-seq to statically link against the sequential version because that also might be useful. Both features use the LP64 interface that lapack also uses. I've also added documentation guiding people to use the intel-mkl-src crate explicitly if they require different linkage. Please let me know what you think.

[geo-ant]

Hi @IvanUkhov, just checking in if I can do anything to help this PR move on.

[geo-ant]

Hi, I have another question: I saw you changed the edition to 2024, I was wondering if it would be okay to revert to 2021. I had a similar request on one of my own crates, since the 2024 edition will require relatively recent compilers to build. Some people are still stuck on older compilers, but 2021 seems to generally work.

[IvanUkhov]

Can you please rebase on main? The diff is a bit confusing.

[IvanUkhov]

Just for clarity, by rebasing I did not mean to merge main into your branch but to actually rebase with git rebase. If it was rebased on main, the highlighted commits would appear to come out of the tip of the main branch.

To achieve that, you need to go to your copy on GitHub and click the button for syncing with the upstream (this repository). That button should appear somewhere here:

Once your main is up to date, you can check it out locally and pull so that it is up to date locally, too. Then you check out your feature/allow-specifying-mkl-linkage and do git rebase main. But at this point, I suspect you would run into a lot of merge conflicts. So it is more of a note for future.

[geo-ant]

Just for clarity, by rebasing I did not mean to merge main into your branch but to actually rebase with git rebase. If it was rebased on main, the highlighted commits would appear to come out of the tip of the main branch.

To achieve that, you need to go to your copy on GitHub and click the button for syncing with the upstream (this repository). That button should appear somewhere here:

Once your main is up to date, you can check it out locally and pull so that it is up to date locally, too. Then you check out your feature/allow-specifying-mkl-linkage and do git rebase main. But at this point, I suspect you would run into a lot of merge conflicts. So it is more of a note for future.

Thanks, yeah I messed that one up. I'll have to brush up on the git rebase workflow.

[IvanUkhov]

For my understanding, let me try to talk through the issue so you can correct me where I am wrong.

Let us, for instance, take dgesvd from lapack:

https://github.com/blas-lapack-rs/lapack/blob/main/src/lapack-sys.rs#L7055

It uses i32 for lwork. Internally, it calls dgesvd_ defined in lapack-sys where lwork is defined as c_int:

https://github.com/blas-lapack-rs/lapack-sys/blob/main/src/lapack.rs#L3705

In turn, c_int is taken from core::ffi:

https://doc.rust-lang.org/core/ffi/type.c_int.html

The documentation tells us that it is always 32 bits (ignoring “esoteric systems”). From this, we conclude that whatever we compile and try to invoke has to have ints of 32 bits. In particular, for the MKL to play well with the rest of the ecosystem, it has to be compiled with LP64. Hence, technically, we do not want the user to enable ILP64.

Is the above an adequate description?

[geo-ant]

Is the above an adequate description?

Yes, this is exactly my understanding. For 'lapack-src' to play nice with lapack, only the LP64 interface should be exposed.

[IvanUkhov]

I think I now understand how one should be thinking about this. If you insist on having more than one option, we will have to expose all four that are compatible, and each one should be fairly treated. So let us remove intel-mkl-src in favor of these:

• intel-mkl-src-dynamic-parallel,
• intel-mkl-src-dynamic-sequential,
• intel-mkl-src-static-parallel, and
• intel-mkl-src-static-sequential.

As mentioned earlier, ideally, we would want to just have intel-mkl-src and let the user activate features by directly depending on intel-mkl-src, but its default value will not work with the rest of the ecosystem. So we cannot leaving in its default state. I wish intel-mkl-src had actually exposed these six features instead: ilp64, lp64, static, dynamic, parallel, and sequential with errors from the build script when things do not combine well, like when activating the first two together. We could then just activate lp64 here to ensure it works and leave the rest to the user.

Let us also please remove the paragraphs you added to the readme and lib.rs and simply add these four options to the list in those two spaces. You can also bump the version number, as it is mentioned there. I will do the same for blas-src.

I will maybe open an issue in intel-mkl-src to see how the author thinks about this situation.

[geo-ant]

Yes, I'd very much like to have more than one option and I agree with everything you said. I'll take care of it in the coming days.

[IvanUkhov]

Thank you and sorry that it took so long!

[geo-ant]

Thank you as well and no worries at all.