add SVML patch for LLVM 14

[h-vetinari]

Addresses #123 for LLVM 14 (not up to main, because numba isn't ready for that yet and won't be soon).

Patch taken from here.

[conda-forge-linter]

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 (recipe) and found it was in an excellent condition.

[h-vetinari]

https://github.com/numba/llvmlite/pull/830 has been merged today. I presume this PR is still useful/relevant, or will be for the next release... CC @conda-forge/llvmlite @isuruf @Hardcode84

[gmarkall]

cc @stuartarchibald / @sklam

[gmarkall]

I think this is still needed. I see it is currently used in Numba's llvmdev recipe:

https://github.com/numba/llvmlite/blob/f368d79f49a267b779e4ed544d6abdaede6a6327/conda-recipes/llvmdev/meta.yaml#L19

Numba's recipe also tests with the numba-3016.ll file:

https://github.com/numba/llvmlite/blob/f368d79f49a267b779e4ed544d6abdaede6a6327/conda-recipes/llvmdev/build.sh#L100

Rather than deleting the file in this PR, should the test be reinstated instead? It is presently disabled in the build.sh:

https://github.com/conda-forge/llvmdev-feedstock/blob/cc208b656185ff4838158fd0c72f282cc6e605f4/recipe/build.sh#L57

and bld.bat:

https://github.com/conda-forge/llvmdev-feedstock/blob/cc208b656185ff4838158fd0c72f282cc6e605f4/recipe/bld.bat#L34

[conda-forge-webservices[bot]]

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 (recipe) and found it was in an excellent condition.

[h-vetinari]

Hi @gmarkall

Rather than deleting the file in this PR, should the test be reinstated instead?

Absolutely, I re-enabled that test now and also included your commit from #209 (to avoid conflicts one way or another).

[stuartarchibald]

@gmarkall @h-vetinari I think that the "SVML patch" from the llvmlite PR mentioned in the OP will indeed be needed to enable SVML support in Numba. The recently released Numba 0.57 depends on an llvmlite ideally built against an LLVM 14 with the SVML patches present. I think this "ideal" makes this PR relevant/necessary.

[h-vetinari]

Can someone ask for a review for these patches on the llvm mailing list?

There's no mailing list anymore, there's only discourse now.

How was this done for the last round of SVML patches? I'm guessing this will also raise questions about why this isn't being upstreamed etc.?

[h-vetinari]

IOW, I could open a thread on discourse, but I feel like I'm lacking quite a bit of context that would make it possible to ask for this intelligently (& hopefully successfully).

[h-vetinari]

Can someone ask for a review for these patches on the llvm mailing list?

How was this done for the last round of SVML patches? I'm guessing this will also raise questions about why this isn't being upstreamed etc.?

[...] I could open a thread on discourse, but I feel like I'm lacking quite a bit of context that would make it possible to ask for this intelligently (& hopefully successfully).

@gmarkall @stuartarchibald @sklam Could you open that request or provide some context so I can do it?

[h-vetinari]

Gentle ping @gmarkall @stuartarchibald @sklam

[esc]

Gentle ping @gmarkall @stuartarchibald @sklam

They are very busy, but I managed to ask @stuartarchibald in an OOB conversation -- the thing is that these patches were not created by the Numba team, so we are probably not the best ones to try to upstream them.

[h-vetinari]

Thanks for asking @esc. I don't think the bar here is to upstream the patches, but to ask the request for review (to a release that's not supported anymore for almost a year) in a way that's successful. I don't feel I'm in a good position to do so, as I couldn't even say why or how llvmlite requires the SVML patches (other than for performance considerations, presumably)

Also, AFAICT this is necessary for numba/llvmlite in conda-forge, so not exactly a detail.

[esc]

Thanks for asking @esc. I don't think the bar here is to upstream the patches, but to ask the request for review (to a release that's not supported anymore for almost a year) in a way that's successful. I don't feel I'm in a good position to do so, as I couldn't even say why or how llvmlite requires the SVML patches (other than for performance considerations, presumably)

Also, AFAICT this is necessary for numba/llvmlite in conda-forge, so not exactly a detail.

Ah, OK, so you want someone to review the patch as part of this PR?

[h-vetinari]

@esc: Ah, OK, so you want someone to review the patch as part of this PR?

All I'm trying to address are the requested changes above, specifically [hyperlink added by me]:

@isuruf: Can someone ask for a review for these patches on the llvm mailing list?

That's something that I think someone from the numba/llvmlite team should do - ideally whoever updated the patch for LLVM 14.

[esc]

@esc: Ah, OK, so you want someone to review the patch as part of this PR?

All I'm trying to address are the requested changes above, specifically [hyperlink added by me]:

@isuruf: Can someone ask for a review for these patches on the llvm mailing list?

That's something that I think someone from the numba/llvmlite team should do - ideally whoever updated the patch for LLVM 14.

Ah, OK, so what you are trying to do is get someone from Numba to ask someone from LLVM to review the updated SVML Patch for LLVM 14?

[h-vetinari]

Ah, OK, so what you are trying to do is get someone from Numba to ask someone from LLVM to review the updated SVML Patch for LLVM 14?

Yes. Though I'm only reiterating the request that was formulated by @isuruf in this thread, because no-one else was responding.

[esc]

Ah, OK, so what you are trying to do is get someone from Numba to ask someone from LLVM to review the updated SVML Patch for LLVM 14?

Yes. Though I'm only reiterating the request that was formulated by @isuruf in this thread, because no-one else was responding.

I understand now, I will ask about this at the Numba developer meeting today and report back.

[esc]

Ah, OK, so what you are trying to do is get someone from Numba to ask someone from LLVM to review the updated SVML Patch for LLVM 14?

Yes. Though I'm only reiterating the request that was formulated by @isuruf in this thread, because no-one else was responding.

I understand now, I will ask about this at the Numba developer meeting today and report back.

I raised this during the development meeting today and the anecdotal evidence I received was: this patch has been submitted to LLVM several times and in several variants but no-one ever managed to find a reviewer. As such, I think that it is highly unlikely that we will find someone from LLVM to review this patch and thus I don't see the value in directing Numba resources towards this endeavor. I would assume, this is also the reason no response was received for @isuruf 's original request.

[sklam]

I have talked with @Hardcode84 about the SVML patch. It has been waiting for review in LLVM's queue. @Hardcode84 can confirm.

The patch is needed to fix calling convention issues for calling SVML intrinsic in AVX512. IIRC, without the patch, it can cause SIGILL.

FYI, Numba users have uncovered another related issue and we will need to have another LLVM patch soon: https://github.com/numba/llvmlite/issues/943.

[Hardcode84]

Yes, here are some previous attempts https://reviews.llvm.org/D53035 https://reviews.llvm.org/D101253

We may try to push once again, but no concrete timelines yet

[isuruf]

@Hardcode84, do you mind making a post in LLVM discourse asking for a review? I have commit rights in LLVM, but I'm not the best person to review it.

[Hardcode84]

@Hardcode84, do you mind making a post in LLVM discourse asking for a review? I have commit rights in LLVM, but I'm not the best person to review it.

We didn't updated patch to the llvm HEAD yet, but I can try to create post on discourse over next few days.

General plan was to upstream it in 2 parts:

• Upstream custom calling conventions first, this part is pretty isolated from everything else and it should be straightforward.
• Upstream Loop Vectorizer changes (and SLP changes I did for Numba), I expect much more scrutiny here. Also, code there somewhat bitrotten over the years and probably need some additional refactoring and tests.

[h-vetinari]

We didn't updated patch to the llvm HEAD yet, but I can try to create post on discourse over next few days.

If I understand Isuru's request correctly, it's about requesting a review for the patch to LLVM 14. How this ties in with a full rebase to main & upstreaming effort is good context to provide for such a request, but not the blocker here.

[h-vetinari]

FYI, Numba users have uncovered another related issue and we will need to have another LLVM patch soon: numba/llvmlite#943.

I have picked that update into this PR now.

[gmarkall]

It's mentioned in #192 that this is waiting for action from the Numba team - what is required here?

[gmarkall]

Sorry, I mean #209.

[h-vetinari]

It's mentioned in #192 that this is waiting for action from the Numba team - what is required here?

Please just read the thread, it's laid out in plain terms. Isuru requested for someone to ask upstream LLVM to review the patch, and the best-suited person to ask that is either Ivan himself or - barring that - someone else from the numba/llvmlite team (who'd still have more background on this work than me, as for me it is effectively zero).

[gmarkall]

Please just read the thread, it's laid out in plain terms.

I did, but I failed to understand that this PR can't go further until that review request happens (which I think is implied - please do clarify if I've misunderstood).

Isuru requested for someone to ask upstream LLVM to review the patch, and the best-suited person to ask that is either Ivan himself or - barring that - someone else from the numba/llvmlite team (who'd still have more background on this work than me, as for me it is effectively zero).

Many thanks for summarising.

[jakirkham]

@Hardcode84, do you mind making a post in LLVM discourse asking for a review? I have commit rights in LLVM, but I'm not the best person to review it.

We didn't updated patch to the llvm HEAD yet, but I can try to create post on discourse over next few days.

@Hardcode84 have you had a chance to create this post on LLVM's Discourse?

For more context, think maintainers here are asking for your help as maintaining these patches in this build often is at the edge of (if not outside) their experience (or comfort level). Particularly as these patches get rebased over time (and conflicts naturally arise raising questions as to how best to address them)

Reading over your comment above, I get the impression that this is a pain that you are somewhat familiar with (though you seem to have more expertise in resolving these matters)

In any event, thought this contextualization of this request might help clarify why people are asking about upstreaming. Thanks again for your help! 🙏

[Hardcode84]

Sorry for delay,

I've created thread about upstreaming https://discourse.llvm.org/t/x86-finalizing-svml-support-in-llvm/70977 a few days ago, but it didn't get any comments so far.

[jakirkham]

Not at all. We are all busy people 🙂

Thank you! 🙏

[Hardcode84]

Can we reactivate SVML in Numba/llvmlite with patched llvm for the time being? I do not expect fast progress on llvm upstream.

Thanks

[h-vetinari]

@isuruf, can you remove your hold here? I don't think it's realistic to get an upstream review for old branches; I'd be fine with trusting Ivan (which is AFAICT how this was done previously as well).

[isuruf]

I don't think it's realistic to get an upstream review for old branches;

I didn't ask for that. I asked for getting a review for the main branch. As it stands, the LLVM reviews are always on older branches and there's no chance of getting those merged because they are on old branches. I think it would be good to have them target main so that we can get these patches in some day.

[isuruf]

We can get this PR merged in the mean time, but we are back to square one after the merge.

[h-vetinari]

@isuruf: I didn't ask for that. I asked for getting a review for the main branch. [...] I think it would be good to have them target main so that we can get these patches in some day.

That really wasn't clear from the line:

@isuruf: Can someone ask for a review for these patches on the llvm mailing list?

"mailing list" != "phabricator", "review" != "get these patches in", and "these patches" in the context of a PR to 14.x are not self-evidently referring to main (by way of explanation of why I understood something very different; though the interpretation of your request also caused a lot of discussion upthread and you could have clarified that at any point).

I mean, it's very clearly in the interest of llvmlite/numba to not have to keep rebasing that patch all the time. Ivan has open, unreviewed PRs (and now an RFC for how to progress with no responses), but I don't see how that relates to this feedstock.

Naturally, I would also like to have the patches upstreamed, but as long as that's not happening, we can just take the llvmlite patches. I see no point in making this dependent on upstream review, other than to force the llvmlite team to prioritise upstreaming (needless to say it's not up to us to decide their priorities). Of course, we're also not forced to take their patch, but this was the balance so far.

I don't know what the most effective tactic for upstreaming the patches is, but I suspect that some fresh (or at least rebased) PRs would help.

[github-actions[bot]]

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

• linter: passed
• azure: passed

Thus the PR was passing and merged! Have a great day!

[xaleryb]

@h-vetinari I see llvmdev, llvm and libllvm14 were re-build and published on conda-forge channel 4 days ago. Do you have a plan to make a rebuild for llvmlite and publish to conda-forge channel?

[h-vetinari]

Do you have a plan to make a rebuild for llvmlite and publish to conda-forge channel?

Not personally, but now all the preconditions should be met so that @conda-forge/llvmlite can do it (or someone else raises a PR on that feedstock).

[jakirkham]

As the conda-forge build of llvmlite is dynamically linked to LLVM, things should just work

Though please file an llvmlite issue if that is not the case for some reason and we can discuss

[Hardcode84]

As the conda-forge build of llvmlite is dynamically linked to LLVM, things should just work

I think, we need to explicitly rebuild llvmlite, as it checks for SVML during native part cmake configuration https://github.com/numba/llvmlite/blob/main/ffi/CMakeLists.txt#L30

[xaleryb]

@jakirkham issue for llvmlite created, could you please help to speed-up resolution somehow?

[jakirkham]

I'm sorry, but I don't have much bandwidth for this. If you are able to carry it forward, that would be helpful. Can review a PR