IR Module #11

Pangoraw · 2023-05-16T20:25:35Z

This PR adds a module to make working with MLIR more convenient. As an example, I added a simple conversion pipeline from Julia SSAIR to MLIR (which we might not want to keep as a test) for very simple scalar code using a subset of julia intrinsics. But is seems to produce valid code which can be tested using the execution engine!

This is a draft because it may be better to make the API look a bit more like LLVM.jl, notably regarding ownership which is currently modeled as an owned::Bool field on the relevant structs (Region, Block,...).

The code for the IR module mostly comes from Coil.jl.

lib/14/libMLIR_h.jl

vchuravy · 2023-05-16T20:32:25Z

src/Dialects.jl

+
+for fname in (:xori, :andi, :ori)
+    @eval function $fname(context, operands, type=IR.get_type(first(operands)); loc=Location(context))
+        state = OperationState($(string("arith.", fname)), loc)


Could we use TableGen to generate these?

src/IR.jl

vchuravy · 2023-05-16T20:57:01Z

@Pangoraw thanks! That looks like a great starting point.

@femtomc do you have some time to take a look if this jams with your understanding of things?

vchuravy · 2023-05-16T21:00:30Z

Maybe we can break src/IR.jl into multiple files? An mirror the include/mlir-c?

maleadt · 2023-05-17T11:30:30Z

src/IR.jl

+    Location(context, string(lin.file), lin.line)
+Location(context::Context, ::Nothing) = Location(context)
+
+Base.convert(::Type{MlirLocation}, location::Location) = location.location


Shouldn't all these conversions be unsafe_convert methods?

src/IR.jl

vchuravy · 2023-05-17T16:22:39Z

src/IR.jl

+Location(context::Context, lin::Core.LineInfoNode) =
+    Location(context, string(lin.file), lin.line)
+Location(context::Context, lin::LineNumberNode) =
+    isnothing(lin.file) ?
+    Location(context) :
+    Location(context, string(lin.file), lin.line)


Location(context::Context, lin::Core.LineInfoNode)

and

Location(context::Context, lin::LineNumberNode)

Those seem like something we should leave up to the user, or defined in an Interop module

vchuravy · 2023-05-17T16:24:46Z

src/IR.jl

+    operation
+end
+
+get_location(operation) = Location(API.mlirOperationGetLocation(operation))


location, name/nameof seem more idiomatic instead of get_

vchuravy · 2023-05-17T16:25:36Z

src/IR.jl

+function lose_ownership!(operation::Operation)
+    @assert operation.owned
+    @atomic operation.owned = false
+    operation
+end


Can you explain the idea behind these ownership semantics a bit more?

Several wrapper structs are made mutable (Region, Block, Operation) so that an associated finalizer can destroy their associated Mlir{Region,Block,Operation} type (ex using mlirOperationDestroy). But when the block/region/operation is added to a container (Region to an Operation, Block to a Region, or Operation to a Block), the ownership is transfered to this container and the owned field is set to false on the wrapper to prevent running the destroy operation in the finalizer since the container is now in charge of destroy its contained elements.

julia> block = Block(); julia> block = nothing # block finalizer will destroy the MlirBlock julia> region = Region(); block = Block(); julia> push!(region, block); # block.owned is no longer true since region is now responsible for destroying the MlirBlock julia> region = nothing # it is unsafe to use the block after this.

Note that this does not model the fact that the Context struct must outlive all structs created from it. I currently added a context field only to the MModule (maybe to rename MLIRModule) so that one can create a module with a context a keep a reference only to the module. This would not be valid for a Location for example.

julia> context = Context(); julia> location = Location(context); julia> context = nothing; # location should not be used after this

codecov-commenter · 2023-05-19T16:07:54Z

Codecov Report

Patch coverage: 40.59% and project coverage change: -42.20% ⚠️

Comparison is base (75776fa) 83.33% compared to head (812f803) 41.13%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #11       +/-   ##
===========================================
- Coverage   83.33%   41.13%   -42.20%     
===========================================
  Files           1        5        +4     
  Lines           6      581      +575     
===========================================
+ Hits            5      239      +234     
- Misses          1      342      +341

Files Changed	Coverage Δ
src/MLIR.jl	`85.71% <ø> (+2.38%)`	⬆️
src/IR/Pass.jl	`26.25% <26.25%> (ø)`
src/IR/IR.jl	`34.64% <34.64%> (ø)`
src/Dialects.jl	`76.92% <76.92%> (ø)`
src/IR/Support.jl	`83.60% <83.60%> (ø)`

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

jumerckx

I've written a patch to handle context the same way LLVM.jl does (I didn't touch the ownership stuff though). If desirable, I can make a PR to this branch, or include it in a new pr?

src/IR.jl

Co-authored-by: jumerckx <31353884+jumerckx@users.noreply.github.com>

Pangoraw · 2023-08-13T10:19:31Z

Thanks for the review @jumerckx!

I've written a patch to handle context the same way LLVM.jl does (I didn't touch the ownership stuff though). If desirable, I can make a PR to this branch, or include it in a new pr?

That means something like passing a function to the Context constructor like so (I am unfortunately not really familiar with LLVM.jl) ?

Context() do ctx
  # use ctx
end

Can you open a PR targeting this branch?

jumerckx · 2023-08-13T20:09:00Z

That means something like passing a function to the Context constructor like so (I am unfortunately not really familiar with LLVM.jl) ?
Context() do ctx
  # use ctx
end

Yes, and functions internally call context() to get the most recent context on a stack. (See here in LLVM.jl)

Can you open a PR targeting this branch?

Opened one here: #17 , but still a bit of work left to finish it completely (see notes in pr). Depending on if there's additional comments, I'll try to finish it up soon :)

vchuravy · 2023-09-29T13:38:40Z

I will land this PR and we can continue working on this on main?

vchuravy reviewed May 16, 2023

View reviewed changes

lib/14/libMLIR_h.jl Outdated Show resolved Hide resolved

vchuravy reviewed May 16, 2023

View reviewed changes

src/IR.jl Outdated Show resolved Hide resolved

vchuravy reviewed May 16, 2023

View reviewed changes

src/IR.jl Outdated Show resolved Hide resolved

Pangoraw added 5 commits May 16, 2023 22:49

wip: add ir module

773aab4

Make toy brutus work for both 14 & 15

d438436

skip :code_coverage_effect

b158dd5

remove GC.@preserve ref && MlirStringRef handling

0e70ccf

use Base.RefValue for MlirOperationState

34ea3e6

Pangoraw force-pushed the pb/ir_api branch from 53507af to 34ea3e6 Compare May 16, 2023 20:57

maleadt reviewed May 17, 2023

View reviewed changes

vchuravy reviewed May 17, 2023

View reviewed changes

src/IR.jl Outdated Show resolved Hide resolved

vchuravy reviewed May 17, 2023

View reviewed changes

src/IR.jl Outdated Show resolved Hide resolved

vchuravy reviewed May 17, 2023

View reviewed changes

Update to create_operation API instead of exposing OperationState

d352781

jumerckx reviewed Aug 12, 2023

View reviewed changes

src/IR.jl Outdated Show resolved Hide resolved

src/IR.jl Outdated Show resolved Hide resolved

Pangoraw and others added 3 commits August 13, 2023 11:55

Fix boundscheck

781f688

Co-authored-by: jumerckx <31353884+jumerckx@users.noreply.github.com>

Add parse(::Type{MLIRType}, ::Context, s)

ef31356

Co-authored-by: jumerckx <31353884+jumerckx@users.noreply.github.com>

Refactor in files to mimic mlir-c/include/

812f803

jumerckx mentioned this pull request Aug 13, 2023

add LLVM.jl-like context state handling. #17

Merged

vchuravy marked this pull request as ready for review September 29, 2023 13:38

vchuravy merged commit 0a447e6 into main Sep 29, 2023

vchuravy deleted the pb/ir_api branch September 29, 2023 13:39

vchuravy restored the pb/ir_api branch September 29, 2023 13:40

Pangoraw deleted the pb/ir_api branch January 12, 2024 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IR Module #11

IR Module #11

Pangoraw commented May 16, 2023 •

edited

Loading

vchuravy May 16, 2023

vchuravy commented May 16, 2023

vchuravy commented May 16, 2023

maleadt May 17, 2023

vchuravy May 17, 2023 •

edited

Loading

vchuravy May 17, 2023

vchuravy May 17, 2023

Pangoraw May 19, 2023

codecov-commenter commented May 19, 2023 •

edited

Loading

jumerckx left a comment •

edited

Loading

Pangoraw commented Aug 13, 2023

jumerckx commented Aug 13, 2023

vchuravy commented Sep 29, 2023

IR Module #11

IR Module #11

Conversation

Pangoraw commented May 16, 2023 • edited Loading

vchuravy May 16, 2023

Choose a reason for hiding this comment

vchuravy commented May 16, 2023

vchuravy commented May 16, 2023

maleadt May 17, 2023

Choose a reason for hiding this comment

vchuravy May 17, 2023 • edited Loading

Choose a reason for hiding this comment

vchuravy May 17, 2023

Choose a reason for hiding this comment

vchuravy May 17, 2023

Choose a reason for hiding this comment

Pangoraw May 19, 2023

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2023 • edited Loading

Codecov Report

jumerckx left a comment • edited Loading

Choose a reason for hiding this comment

Pangoraw commented Aug 13, 2023

jumerckx commented Aug 13, 2023

vchuravy commented Sep 29, 2023

Pangoraw commented May 16, 2023 •

edited

Loading

vchuravy May 17, 2023 •

edited

Loading

codecov-commenter commented May 19, 2023 •

edited

Loading

jumerckx left a comment •

edited

Loading