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

Enable OpaquePointers on LLVM 15 #49846

Closed
wants to merge 1 commit into from
Closed

Enable OpaquePointers on LLVM 15 #49846

wants to merge 1 commit into from

Conversation

vchuravy
Copy link
Member

No description provided.

@vchuravy
Copy link
Member Author

@nanosoldier runbenchmarks(!"scalar", vs=":master")

@ViralBShah ViralBShah added the compiler:codegen Generation of LLVM IR and native code label May 16, 2023
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@maleadt
Copy link
Member

maleadt commented May 17, 2023

Opaque pointers are a per-context setting, right? It would be nice if the GPU stack can keep working with opaque pointers disabled, as we have a couple of (closed-source) back-ends that haven't been adapted yet.

@Seelengrab
Copy link
Contributor

If this becomes a flag in GPUCompiler, it would be great to have some API for setting that as well.

@maleadt
Copy link
Member

maleadt commented May 17, 2023

LLVM.jl has a wrapper for the LLVM C API already: https://github.com/maleadt/LLVM.jl/blob/e44c430c4a48225114f217c36b166b85a3fcbe5a/lib/15/libLLVM_h.jl#L602-L604. Now that GPUCompiler create its own contexts for use with Julia, theoretically this should just work, but we should verify.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 18, 2023

@nanosoldier runbenchmarks("union", vs=":master")

@vtjnash vtjnash marked this pull request as ready for review October 18, 2023 18:48
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vchuravy
Copy link
Member Author

@pxl-th @maleadt do you think the GPU ecosystem is at a point where turning opaque-pointer should be fine?

@vtjnash and I are inclined on giving this a go.

@pxl-th
Copy link

pxl-th commented Oct 21, 2023

Last time I tried opaque pointers it needed some adjustments on GPUCompiler side:
JuliaGPU/GPUCompiler.jl#511.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 21, 2023

I changed this PR so it only turns them on for the JIT and not globally.

@maleadt
Copy link
Member

maleadt commented Oct 21, 2023

The main problem isn't GPUCompiler, it's the out-of-tree back-ends (SPIR-V translator for oneAPI, the AIR "back-end" for Metal, and NVVM for CUDA). Updating these is going to be a pain regardless, so it shouldn't hold up the migration here. I just was hoping that the next LTS would still support typed pointers, so that we have something to point users to while we're updating back-ends.

@maleadt
Copy link
Member

maleadt commented Oct 24, 2023

I've been testing this, and it works great. I wonder if we could have the NFC version of this in 1.10, i.e., calling setOpaquePointers instead of using the global option. That way, we can start migrating GPUCompiler even if Julia is still using typed pointers (currently, using the global option "taints" the context, making it impossible to change the setting afterwards).
EDIT: see #51840

src/jitlayers.cpp Outdated Show resolved Hide resolved
@maleadt
Copy link
Member

maleadt commented Oct 26, 2023

For the record, with #51840 it doesn't matter whether Julia uses opaque pointers or not (LLVM's default is still typed, and Julia doesn't use the clopt anymore allowing GPUCompiler to override per-context). But I've also started work on updating GPUCompiler to opaque pointers in JuliaGPU/GPUCompiler.jl#528. Regardless, it should be fine to flip the switch here.

Let's see what else breaks though:

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@vchuravy
Copy link
Member Author

vchuravy commented Dec 4, 2023

Will fix #52363 on for 1.11-dev

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Dec 12, 2023
@gbaraldi
Copy link
Member

Opaque pointers introduce a bunch of regressions that are only fixed on a patched llvm 16

@gbaraldi gbaraldi removed the merge me PR is reviewed. Merge when all tests are passing label Dec 12, 2023
@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 12, 2023

Did it get worse? The results above seemed to look fine
@nanosoldier runbenchmarks(!"scalar", vs=":master")

@gbaraldi
Copy link
Member

Unions are on average 50% slower

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vchuravy
Copy link
Member Author

Superseded by #51720

@vchuravy vchuravy closed this Jan 22, 2024
@vchuravy vchuravy deleted the vc/opaque_pointers branch January 22, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants