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

Support building against llvm15 #1035

Merged
merged 11 commits into from
Mar 30, 2024
Merged

Support building against llvm15 #1035

merged 11 commits into from
Mar 30, 2024

Conversation

yashssh
Copy link
Contributor

@yashssh yashssh commented Mar 13, 2024

Add support for llvmlite to be built against llvm15 while keeping support for llvm14 as well.

1) Drop 2 passes no longer supported by old PM
2) Update domTree passes to their wrapper passes
3) Update getAddress() api
4) Disable opaque pointer mode
5) Give warning when build with llvm15

All the changes are put inside llvm version check guard so that it builds with both llvm14 and llvm15

yashssh added 2 commits March 12, 2024 16:41
1) Drop 2 passes no longer supported by old PM
2) Update domTree passes to their wrapper passes
3) Update getAddress() api
4) Disable opaque pointer mode
5) Give warning when build with llvm15

All the changes are put inside llvm version check guard
so that it builds with both llvm14 and llvm15
@yashssh
Copy link
Contributor Author

yashssh commented Mar 13, 2024

Removing draft tag as it won't allow me to add reviewers. Adding "draft" to the title instead.

Edit: Turns out I can't add reviewers. @gmarkall please add yourself and other reviewers who might be interested in this.

@yashssh yashssh marked this pull request as ready for review March 13, 2024 17:13
@yashssh yashssh changed the title Support llvmlite to building against llvm15 [Draft] Support llvmlite to building against llvm15 Mar 13, 2024
ffi/Makefile.linux Outdated Show resolved Hide resolved
ffi/core.cpp Outdated
LLVMPY_GetGlobalContext() {
auto context = LLVMGetGlobalContext();
#if LLVM_VERSION_MAJOR > 14
LLVMContextSetOpaquePointers(context, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to other reviewers - this does result in opaque pointers being disabled for the global context multiple times, but this operation is idempotent.

@@ -2,6 +2,11 @@
#define LLVMPY_CORE_H_

#include "llvm-c/Core.h"

// Needed for macros that control version-specific behaviour - included here so
// that they are available in all ffi translation units
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular, this defines the LLVM_VERSION_MAJOR macro referenced in various places in this PR.

@@ -653,7 +654,7 @@ def break_up_asm(self, asm):
def test_rv32d_ilp32(self):
self.check_riscv_target()
llmod = self.fpadd_ll_module()
target = self.riscv_target_machine(features="+f,+d")
target = self.riscv_target_machine(features="+f,+d", abiname="ilp32")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM 14 defaults to the soft-float ABI, LLVM 15 defaults to the hard-float ABI. We explicitly specify the soft-float ABI for consistency with the expected output. Note that hard float and hard double ABIs are tested in the subsequent two tests below.

self.assertIn(patch, range(10))
valid = (14, 15)
self.assertIn(major, valid)
self.assertIn(patch, range(8))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Max patch versions are 14.0.6 and 15.0.7)

@gmarkall gmarkall changed the title [Draft] Support llvmlite to building against llvm15 Support building against llvm15 Mar 13, 2024
This includes changes:

- Adding the Windows LLVM 15 config to the Azure setup.
- Converting the commands in the Azure pipeline file to a call to the
  setup_conda_environment.cmd script (it's easier to edit a script
  rather than individual lines in the Azure file).
- Installation of LLVM 15 from conda-forge.
- A workaround / supporting change for the conda-forge solution to
  conda-forge/llvmdev-feedstock#175.
gmarkall
gmarkall previously approved these changes Mar 14, 2024
Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes here have my approval - I've already done a couple of rounds of review internally on this PR, and I made a few edits for a couple of left over changes that needed removing (adjustments to the Makefile, and a test that was accidentally skipped).

I added the CI config so we can test with LLVM 15 from conda-forge on each platform - someone else might want to review those changes.

I ran the Numba test suite with this PR locally, and didn't encounter any issues - the two passes that aren't available with the legacy pass manager anymore (arg promotion and loop unswitch) don't seem to be used by Numba.

As I see it, this PR provides us a way to give experimental support for LLVM 15 once merged, with no further blockers. Following that, I think we can:

  • Build an LLVM 15 llvmdev package and move testing to LLVM 15 exclusively, with the aim of officially moving to 15.
  • Move to the new pass manager, and re-enable the passes that have to be disabled with LLVM 15 at present.
  • Switch to opaque pointers (at least in the binding layer)
  • Move to LLVM 16+

Some discussion of the ordering / dependencies of these is warranted though - perhaps at the next office hours.

Copy link

@modiking modiking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this up!

@@ -161,8 +161,13 @@ LLVMPY_AddCallGraphDOTPrinterPass(LLVMPassManagerRef PM) {

API_EXPORT(void)
LLVMPY_AddDotDomPrinterPass(LLVMPassManagerRef PM, bool showBody) {
#if LLVM_VERSION_MAJOR <= 14

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest being consistent with the #ifs, either all "<= 14" or all "< 15"

@@ -333,12 +345,14 @@ LLVMPY_AddLoopUnrollAndJamPass(LLVMPassManagerRef PM) {
LLVMAddLoopUnrollAndJamPass(PM);
}

#if LLVM_VERSION_MAJOR < 15
API_EXPORT(void)
LLVMPY_AddLoopUnswitchPass(LLVMPassManagerRef PM, bool optimizeForSize,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference the loop unswitch pass has been renamed to SimpleLoopUnswitchPass in the new pass manager (https://reviews.llvm.org/D124376). Naturally to enable this we need to move to NPM so it makes sense that this is disabled for now but it can come back with NPM.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually createSimpleLoopUnswitchLegacyPass still exists in LLVM-15 so it can be dropped in here without issue.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API_EXPORT(void)
LLVMPY_AddLoopUnswitchPass(LLVMPassManagerRef PM, bool optimizeForSize,
                           bool hasBranchDivergence) {
#if LLVM_VERSION_MAJOR < 15
    unwrap(PM)->add(
        createLoopUnswitchPass(optimizeForSize, hasBranchDivergence));
#else
    unwrap(PM)->add(
        createSimpleLoopUnswitchLegacyPass(optimizeForSize));
#endif
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding this, I have updated the code.

@@ -244,10 +254,12 @@ LLVMPY_AddAlwaysInlinerPass(LLVMPassManagerRef PM, bool insertLifetime) {
unwrap(PM)->add(llvm::createAlwaysInlinerLegacyPass(insertLifetime));
}

#if LLVM_VERSION_MAJOR < 15
API_EXPORT(void)
LLVMPY_AddArgPromotionPass(LLVMPassManagerRef PM, unsigned int maxElements) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto with ArgumentPromotionPass in NPM as below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean legacy version of this pass is available as well? I couldn't find it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I meant the new pass version, I wrote both of these comments together then saw the legacy pass still existed.

On that note, the NPM setup slots in differently (using pipeline tuning options) so we can’t replace these in-situ. No needed for this change though.

Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving following latest changes.

@modiking
Copy link

LGTM

@sklam sklam self-requested a review March 19, 2024 14:41
@sklam sklam added the Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm label Mar 19, 2024
@gmarkall
Copy link
Member

Added "Pending Buildfarm" to make sure this is all OK on all other platforms.

@sklam
Copy link
Member

sklam commented Mar 29, 2024

BFID: llvmlite_275

@sklam
Copy link
Member

sklam commented Mar 29, 2024

llvmlite_275 passed

@sklam
Copy link
Member

sklam commented Mar 30, 2024

Also passed buildfarm test against numba main (numba_yaml_448)

@sklam sklam added BuildFarm Passed For PRs that have been through the buildfarm and passed and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm labels Mar 30, 2024
@sklam sklam merged commit 42b9834 into numba:main Mar 30, 2024
23 checks passed
modiking added a commit to modiking/llvmlite that referenced this pull request Apr 24, 2024
@sklam sklam added this to the v0.43.0-rc1 milestone Apr 29, 2024
This was referenced May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants