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

MLIRValueTrait to support user defined Value types #64

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jumerckx
Copy link
Collaborator

What this enables:

# Setup #
using MLIR
using MLIR: IR, API
using MLIR.IR: Value, Operation, Block, push_argument!, Context, MLIRValueTrait, Convertible
using MLIR.Dialects: arith
ctx = Context()

# Define a Julia type and mapping to MLIR #
struct MLIRInteger{N} <: Integer
    value::Value
    MLIRInteger{N}(i::Value) where {N} = new(i)
end
const i64 = MLIRInteger{64}
IR.Type(::Type{MLIRInteger{N}}) where {N} = API.mlirIntegerTypeGet(ctx, N)
MLIRValueTrait(::Type{<:MLIRInteger}) = Convertible()

# Specialize or write function #
function Base.:+(a::T, b::T)::T where {T<:MLIRInteger}
    T(IR.result(arith.addi(a, b)))
end

# Execute # 
block = Block()
a = i64(push_argument!(block, IR.Type(i64)))
b = i64(push_argument!(block, IR.Type(i64)))

a+b

Of course, using this approach you lose direct access to a handle to the generated operation (arith.addi) so it is of dubious value as of now.
In Jojo.jl this system is used in combination with CassetteOverlay to make sure that all created operations are collected. This system is still a bit finicky and I'm still working on it so I wouldn't try upstream this yet.

While collecting things to push here, it's clear some cleaning up is still required.

  • get_value is a bad name. It could be changed to Value, or instead, use Julia conversion convert(::Value, ...) which would probably be nicer to allow automatic conversion when e.g. pushing to Value[]
  • MLIRValueTrait => ValueTrait?

I'm not even sure if it's necessary to have this trait system. For now, the conversion to IR.Value is so simple that you might as well just specialize get_value instead of letting it happen through the trait. On the other hand, this might help in the future as people want to add extra functionality to Value-like types.

src/IR/Value.jl Show resolved Hide resolved
src/IR/Value.jl Outdated Show resolved Hide resolved
@@ -197,7 +197,7 @@ function {0}({1}location=Location())
end
)"; // 0: functionname, 1: functionarguments, 2: functionbody
const char *functionbodytemplate = R"(results = IR.Type[{0}]
operands = Value[{1}]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use API.MlirValue instead of IR.Value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, I don't it should.

deps/tblgen/jl-generators.cc Outdated Show resolved Hide resolved
deps/tblgen/jl-generators.cc Outdated Show resolved Hide resolved
deps/tblgen/jl-generators.cc Outdated Show resolved Hide resolved
deps/tblgen/jl-generators.cc Outdated Show resolved Hide resolved
jumerckx and others added 3 commits March 20, 2024 21:10
Co-authored-by: Sergio Sánchez Ramírez <[email protected]>
Co-authored-by: Sergio Sánchez Ramírez <[email protected]>
Co-authored-by: Sergio Sánchez Ramírez <[email protected]>
@jumerckx
Copy link
Collaborator Author

Thanks for the changes @mofeing! What do you think of using Base.convert(Value, ...) instead of value? Also, if we don't go with the convert, wouldn't Value (capitalized) make more sense since we're returning an object of that type?

@mofeing
Copy link
Collaborator

mofeing commented Mar 21, 2024

Thanks for the changes @mofeing! What do you think of using Base.convert(Value, ...) instead of value? Also, if we don't go with the convert, wouldn't Value (capitalized) make more sense since we're returning an object of that type?

I like to use constructor methods (i.e. Value) whenever the passing argument has a field from which it can be easily extracted.

The problem with Base.convert is that it has some clear semantics and can be implicitly called on some cases: https://docs.julialang.org/en/v1/manual/conversion-and-promotion/#When-is-convert-called?

I'm not sure in which cases you will need to convert from some other type to Value, but check out the link I posted above and decide based on that.

EDIT: Can you give me some example where you need to convert some other type to Value please?

@jumerckx
Copy link
Collaborator Author

I like to use constructor methods (i.e. Value) whenever the passing argument has a field from which it can be easily extracted.

Isn't this the case here then? The standard assumption is that these value-like types have a field holding an actual IR.Value. And if wouldn't hold for a type, the user would have to implement the Value-getter (whatever it's called) themselves.

EDIT: Can you give me some example where you need to convert some other type to Value please?

Actually, the one example I had one was to get rid of explicit conversions when pushing to the operands array in generated dialect wrappers. e.g.:

function addf(lhs, rhs; result=nothing::Union{Nothing, IR.Type}, fastmath=nothing, location=Location())
    results = IR.Type[]
    operands = IR.Value[lhs, rhs] # instead of IR.Value[value(lhs), value(rhs)]

But upon thinking more about this, that really isn't a good use-case as this code is auto-generated in the first place and the implicit conversion might cause more confusion elsewhere.

Conversion in the other direction could be more interesting, though. For example, an explicit return type for a function would convert whatever result that that function returns automatically. But again, this might cause more confusion, so maybe left best for later down the line in a different pr.

Do we merge this pr once the naming has decided or are there other considerations?

@mofeing
Copy link
Collaborator

mofeing commented Mar 26, 2024

Do we merge this pr once the naming has decided or are there other considerations?

I'm ok with merging as it is but you just need to update the other value method definitions, like

function value(expr::AffineExpr)

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 2.28%. Comparing base (f561fa2) to head (1638254).
Report is 3 commits behind head on main.

Files Patch % Lines
src/IR/Value.jl 0.00% 5 Missing ⚠️
src/IR/AffineExpr.jl 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##            main     #64      +/-   ##
========================================
- Coverage   2.28%   2.28%   -0.01%     
========================================
  Files        125     125              
  Lines      28527   28532       +5     
========================================
  Hits         653     653              
- Misses     27874   27879       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mofeing
Copy link
Collaborator

mofeing commented Jun 1, 2024

I've updated MLIR_jll to also accept v17, so the CI failing on Julia nightly should no longer fail.

Anyway, you're free to merge whenever you want to.

@jumerckx
Copy link
Collaborator Author

jumerckx commented Jun 1, 2024

I've updated MLIR_jll to also accept v17, so the CI failing on Julia nightly should no longer fail.

Thanks for the heads up, and for your CI work! The error seems to be an Pkg environment conflict so not sure what could be causing this as project.toml is untouched?

I kind of lost the plot with this PR. I'm currently prepping a branch with my work on generating MLIR from Julia code that will supersede this one so this shouldn't be merged anymore.

@jumerckx jumerckx marked this pull request as draft June 1, 2024 22:10
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