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.

Sign up for GitHub

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

Jump to bottom

Support for LLVM 15 and opaque pointers #326

Merged
merged 17 commits into from
Mar 22, 2023
Merged

Support for LLVM 15 and opaque pointers #326

merged 17 commits into from
Mar 22, 2023

Conversation

gbaraldi
Copy link
Contributor

@gbaraldi gbaraldi commented Mar 15, 2023 •
edited
Loading

Initial set of changes for LLVM 15
This already passes a good amount of tests, and also doesn't segfault during the tests.

I'm up to bikeshedding how best to make the version checks.

@gbaraldi gbaraldi marked this pull request as ready for review March 17, 2023 19:48
maleadt
maleadt reviewed Mar 20, 2023
View reviewed changes
src/core/function.jl Outdated
@@ -10,6 +10,8 @@ register(Function, API.LLVMFunctionValueKind)
Function(mod::Module, name::String, ft::FunctionType) =
Function(API.LLVMAddFunction(mod, name, ft))

FunctionType(Fn::Function) = FunctionType(API.LLVMGetFunctionType(Fn))
Copy link
Owner

Choose a reason for hiding this comment

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

We can do this already using eltype(llvmtype(fn)). Probably wouldn't hurt to have the FunctionType ctor for visibility, but it doesn't need an additional API call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't do this, at least not with eltype(llvmtype(fn) because

julia> f = LLVM.Function(mod, "f", fun_type)
declare i32 @f(i32, i32)


julia> llvmtype(f)
ptr

and eltype(ptrtype) is illegal

Copy link
Owner

@maleadt maleadt Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh right; that will require a lot of updates across the stack then (we're doing eltype(llvmtype(...)) in a lot of places).

I'm not entirely sold on FunctionType(::Function), as that looks more like a conversion than a type query, but I don't have anything better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

llvmeltype is probably illegal everywhere.

Copy link
Owner

@maleadt maleadt Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Still legal for Arrays, Vectors, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case illegal uses should be caught by that error method. Otherwise it segfaults

Copy link
Contributor Author

@gbaraldi gbaraldi Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe making it function_type(::Function), so it doesn't look like a constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Github shows 122 results for eltype(llvmtype( + 15 llvmeltype. I suspect lots of those are fine, but yeah.

Copy link
Owner

Choose a reason for hiding this comment

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

function_type sounds good. Maybe we should rename llvmtype to value_type then? Or that may be too breaking for what it's worth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

llvmtype isn't too bad, and value_type isn't super clear IMO, and llvm_value_type is a handful.

maleadt
maleadt reviewed Mar 20, 2023
View reviewed changes
test/core.jl Outdated Show resolved Hide resolved
maleadt
maleadt reviewed Mar 20, 2023
View reviewed changes
test/interop.jl Outdated Show resolved Hide resolved
maleadt
maleadt reviewed Mar 20, 2023
View reviewed changes
test/interop.jl Outdated Show resolved Hide resolved
maleadt
maleadt reviewed Mar 20, 2023
View reviewed changes
test/interop.jl Outdated Show resolved Hide resolved
maleadt
maleadt reviewed Mar 20, 2023
View reviewed changes
test/interop.jl Outdated Show resolved Hide resolved
@maleadt maleadt mentioned this pull request Mar 21, 2023
Merged
@maleadt
Copy link
Owner

maleadt commented Mar 21, 2023

We should probably wait to merge this until JuliaLang/julia#48700 has landed. Using that PR, I'm getting an assertion failure:

julia: /workspace/srcdir/llvm-project/llvm/lib/IR/Instructions.cpp:1633: void llvm::AtomicRMWInst::Init(llvm::AtomicRMWInst::BinOp, llvm::Value*, llvm::Value*, llvm::Align, llvm::AtomicOrdering, llvm::SyncScope::ID): Assertion `Ordering != AtomicOrdering::Unordered && "atomicrmw instructions cannot be unordered."' failed.

Actually, let me just have CI use that branch over here.

@gbaraldi
Copy link
Contributor Author

I looked a bit into that failure and am quite confused, we don't use unordered ordering anywhere in the tests.

@maleadt
Copy link
Owner

maleadt commented Mar 21, 2023 •
edited
Loading

Comes from LLVM.jl though:

julia: /workspace/srcdir/llvm-project/llvm/lib/IR/Instructions.cpp:1633: void llvm::AtomicRMWInst::Init(llvm::AtomicRMWInst::BinOp, llvm::Value*, llvm::Value*, llvm::Align, llvm::AtomicOrdering, llvm::SyncScope::ID): Assertion `Ordering != AtomicOrdering::Unordered && "atomicrmw instructions cannot be unordered."' failed.

[130690] signal (6.-6): Aborted
in expression starting at /home/tim/Julia/pkg/LLVM/test/atomics.jl:5
unknown function (ip: 0x7f1c1c8df8ec)
gsignal at /usr/lib/libc.so.6 (unknown line)
abort at /usr/lib/libc.so.6 (unknown line)
unknown function (ip: 0x7f1c1c87a45b)
__assert_fail at /usr/lib/libc.so.6 (unknown line)
_ZN4llvm13AtomicRMWInst4InitENS0_5BinOpEPNS_5ValueES3_NS_5AlignENS_14AtomicOrderingEh at /home/tim/Julia/src/julia/build/dev/usr/bin/../lib/libLLVM-15jl.so (unknown line)
LLVMBuildAtomicRMW at /home/tim/Julia/src/julia/build/dev/usr/bin/../lib/libLLVM-15jl.so (unknown line)
LLVMBuildAtomicRMW at /home/tim/Julia/pkg/LLVM/lib/15/libLLVM_h.jl:3255 [inlined]
atomic_rmw! at /home/tim/Julia/pkg/LLVM/src/irbuilder.jl:278 [inlined]
macro expansion at /home/tim/Julia/pkg/LLVM/src/interop/atomics.jl:294 [inlined]
macro expansion at /home/tim/Julia/pkg/LLVM/src/base.jl:102 [inlined]
macro expansion at /home/tim/Julia/pkg/LLVM/src/interop/atomics.jl:287 [inlined]
macro expansion at /home/tim/Julia/pkg/LLVM/src/base.jl:102 [inlined]
#s936#29 at /home/tim/Julia/pkg/LLVM/src/interop/atomics.jl:279 [inlined]

@gbaraldi
Copy link
Contributor Author

gbaraldi commented Mar 21, 2023 •
edited
Loading

It fails in type inference which is odd.
Commenting out Unordered from that enum seems to make it pass, so I wonder if we are speculating a call into this.

@maleadt
Copy link
Owner

maleadt commented Mar 21, 2023

It fails in type inference which is odd.

It's a generated function that gets expanded by type inference, which calls into LLVM and triggers an assertion while generating IR.

@maleadt
Copy link
Owner

maleadt commented Mar 21, 2023

Looked into the issue, and the @dynamic_order from Interop.atomics generated lots of code to support an atomic ordering specified by symbol (as opposed to something in the type domain), including some invalid combinations that abort now (AtomicRMW + Unordered). Instead of fixing that functionality up, I realized it doesn't really belong here, so I've removed it and filed JuliaConcurrent/UnsafeAtomicsLLVM.jl#7 (which is the only consumer of this functionality).

@maleadt
Copy link
Owner

maleadt commented Mar 22, 2023 •
edited
Loading

Expat_jll broke?

ERROR: LoadError: LoadError: InitError: could not load library "/Users/runner/.julia/artifacts/3bb83a2812f716f0d1610026d1e470dbf92a952e/lib/libexpat.1.8.8.dylib"
dlopen(/Users/runner/.julia/artifacts/3bb83a2812f716f0d1610026d1e470dbf92a952e/lib/libexpat.1.8.8.dylib, 0x0001): tried: '/Users/runner/.julia/artifacts/3bb83a2812f716f0d1610026d1e470dbf92a952e/lib/libexpat.1.8.8.dylib' (no such file)
Stacktrace:
  [1] dlopen(s::String, flags::UInt32; throw_error::Bool)
    @ Base.Libc.Libdl ./libdl.jl:114
  [2] dlopen(s::String, flags::UInt32)
    @ Base.Libc.Libdl ./libdl.jl:114
  [3] macro expansion
    @ ~/.julia/packages/JLLWrappers/QpMQW/src/products/library_generators.jl:54 [inlined]
  [4] __init__()
    @ Expat_jll ~/.julia/packages/Expat_jll/34w3j/src/wrappers/x86_64-apple-darwin.jl:9

Hasn't been touched in ages... Maybe the cache is corrupted?

@gbaraldi
Copy link
Contributor Author

It didn't even find it, so I guess so

@maleadt maleadt changed the title Initial fixes for LLVM15 Support for LLVM 15 and opaque pointers Mar 22, 2023
@maleadt maleadt merged commit 1205905 into maleadt:master Mar 22, 2023
@maleadt
Copy link
Owner

maleadt commented Mar 22, 2023

Thanks for the PR! I want to have this live on master for a bit so that we can iron out any necessary API changes, so I won't tag immediately.

@maleadt maleadt mentioned this pull request Mar 23, 2023
Closed
maleadt added a commit that referenced this pull request Mar 25, 2023
@gbaraldi @maleadt
Support for LLVM 15 and opaque pointers (#326)
00963c6 
Co-authored-by: Tim Besard <[email protected]>
@maleadt maleadt mentioned this pull request Mar 31, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maleadt maleadt maleadt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gbaraldi @maleadt