LLVM new pass manager support

AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers

BSD 3-Clause "New" or "Revised" License

2.05k stars 348 forks source link

LLVM new pass manager support #1729

Closed brechtvl closed 9 months ago

brechtvl commented 10 months ago

Description

LLVM 16 removes support for default optimizations levels (O1, O2, O3) from the legacy pass manager, which are used by OSL. This adds supports for the new pass manager while keeping the legacy pass manager working for older LLVM versions.

Enable new pass manager for LLVM 16+
Both default and custom optimization levels have been implemented
But the performance and correctness beyond passing tests has not been verified
ptx_compile_group still uses the legacy pass manager for linking

Tests

No new tests, the entire testsuite tests this.

Checklist:

[x] I have read the contribution guidelines.
[x] I have previously submitted a Contributor License Agreement.
[x] I have updated the documentation, if applicable.
[x] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
[x] My code follows the prevailing code style of this project.

brechtvl commented 10 months ago

The conservative option here would to only enable this for LLVM 16+, so that anyone already building with LLVM 15 will see no difference. But for CI in this PR it's useful to test at least one LLVM version with the new pass manager.

lgritz commented 10 months ago

I agree, let's not enable for LLVM 15 to avoid any possible breakage for now.

Let's get these reviewed and merged, and then we'll adjust the CI to have a test that builds against 16.

lgritz commented 10 months ago

I think what I said before may not have been clear. Let me try again:

For now, enable this for LLVM 15 so it's well tested in CI. Once all of these PRs get merged, we'll be sure to add LLVM 16 to the CI, and then if we want, we can re-disable LLVM so that it uses the old pass manager and therefore can't break what was working before for people using LLVM <= 15.

lgritz commented 10 months ago

I think this is the next one we should merge. I believe it needs a rebase to double check everything.

brechtvl commented 10 months ago

This one I think we also want to switch it to LLVM 16+ only before merging.

lgritz commented 9 months ago

It seems to pass all the tests. Are you just worried about the risk, not wanting to disrupt the versions that worked before?

brechtvl commented 9 months ago

The main risk I see is that OptiX might break due to the problem mentioned in https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/pull/1730#issuecomment-1738971908, or that performance might be bad since I don't have a good way to test it.

I guess I don't see a good reason to take any risk if we can just add LLVM 16 to CI soon and test the new code that way.

lgritz commented 9 months ago

Your plan is fine with me.

Do you have more to add here, then?

brechtvl commented 9 months ago

I think this is ready to merge now.

lgritz commented 9 months ago

OK, will merge when the CI run completes.