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

Replace Base.libllvm_version with libmlir_version preference #70

Closed
wants to merge 3 commits into from

Conversation

mofeing
Copy link
Member

@mofeing mofeing commented Apr 23, 2024

I didn't put this into #69 because this might require more discussion.

This PR lets the user select the MLIR version they want on compile-time.

Wait, why do this?

Well, several projects experimenting with MLIR.jl, like Coil.jl and Reactant.jl, need to import the whole MLIR.jl code just because they use another libMLIR (included in XLA or IREE). They just need a way to overwrite MLIR_jll.mlir_c.

Since we conditionally load code based on Julia's LLVM version (mainly for MLIR.API and MLIR.Dialects), the loaded code won't match the version of the custom MLIR loaded version.

This PR solves it by letting the user explicitly set the correct MLIR version using Preferences.

Upstreaming a patch for adding a mlir_version function in libMLIR-C would remove the need for this, since the users would just overwrite mlir_c in MLIR_jll and we could ask the version to it. Meanwhile, and for supporting older versions, this patch will do the trick.

@wsmoses
Copy link

wsmoses commented Apr 23, 2024

I'm not quite sure this suffices, since someone else in the julia ecosystem would have their MLIR broken so you could not have a package using mlir.jl and the mlir with different lib at the same time.

@mofeing
Copy link
Member Author

mofeing commented Apr 23, 2024

I'm not quite sure this suffices, since someone else in the julia ecosystem would have their MLIR broken so you could not have a package using mlir.jl and the mlir with different lib at the same time.

This can be solved by setting mlir_c to be a ScopedValue. But the remaining problem is the conditional code loading depending on MLIR version.

IMO we should remove the conditional code load and do some kind of dynamic dispatch based on some runtime MLIR version.

To my mind comes the following idea: Any user who needs to use an external libMLIR should create an VersionDispatcher (see below) and call the API through it. All the calls would be dynamically dispatched, but I think that's not really a problem or at least, it's a price I'm willing to pay.

struct VersionDispatcher
	version::VersionNumber
end

function getproperty(wrapper::VersionDispatcher, key::Symbol)
    if v"15" <= wrapper.version < v"16"
        getproperty(API.v15, key)
    elseif v"16" <= wrapper.version < v"17"
        getproperty(API.v16, key)
    elseif v"17" <= wrapper.version < v"18"
        getproperty(API.v17, key)
    else
        error("Not a valid MLIR version: $(wrapper.version)")
    end
end

# example
dispatcher = VersionDispatcher(v"16")
dispatcher.mlirBlockCreate(...)

Upstreaming a patch for adding a mlir_version function in libMLIR-C would remove the need for this, since the users would just overwrite mlir_c in MLIR_jll and we could ask the version to it. Meanwhile, and for supporting older versions, this patch will do the trick.

If we get this done, then we could do the VersionDispatcher and the ScopedValue change of scope all in one move.

using ScopedValues

struct VersionDispatcher
	version::VersionNumber
    libmlir::String
end

function VersionDispatcher(libmlir::String)
	version = VersionNumber(@ccall libmlir.version()::Cstring)
    VersionDispatcher(version, libmlir)
end

function getproperty(wrapper::VersionDispatcher, key::Symbol)
    with(mlir_c => wrapper.libmlir) do 
	    if v"15" <= wrapper.version < v"16"
	        getproperty(API.v15, key)
	    elseif ...
	        ...
	    end
    end
end

EDIT: Revising the code, I now see that we should be careful with some parts. Specifically type definitions in MLIR.IR submodule (e.g. IR.Block has a field of type API.MlirBlock where API contents are versioned). I don't think it's a blocker because most type definitions in API are hidden behind a Ptr{Cvoid}, but we must move the struct definitions out of API and the binding generator.

@Pangoraw
Copy link
Member

FYI, another reason for Coil not currently using MLIR.jl is that it uses a custom ccall macro to handle the versioned symbols provided by the upstream libIREECompiler. I need to build a libIREECompiler_jll without versionning.

@mofeing
Copy link
Member Author

mofeing commented Apr 24, 2024

Superseeded by #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants