Draft: Dialect generation from ODS #274

Danacus · 2023-08-14T14:37:42Z

I've created an early draft of dialect generation based on the MLIR Python Bindings.

It's okay if you don't have the time to review this. I admit that there's quite a lot of ugly and hard to read code (especially the code dealing with variadic arguments), and it might not be in a state currently where you would want to maintain it within melior, hence I'm marking it as a draft.

Not much changed since my original implementation mentioned in #262, but I got a bit stuck on error handling in my TableGen wrapper library (tblgen-rs) and lost motivation for a while after that. Anyway, I decided to finish what I started, hence I am making this draft.

To build this branch, you might need to set TABLEGEN_160_PREFIX to match MLIR_SYS_160_PREFIX.

I've added the generated dialects in a new dialect_gen module for now, such that they can be compared with the original hand-written bindings in the dialect module.

There are still a few issues:

Some parts of the code are hacky and ugly, and may be hard to read.
Type inference is not always detected (but it should be as good as the Python bindings at least)
Need to add tests (already have them in a separate repository)
Need to fix some issues with the CI

I wanted complete parity with the existing hand-written dialect bindings, but there are some things that aren't generated as nicely. For example, arith::CmpiPredicate is not generated and plain Attribute is used instead for arith::cmpi. It might be feasible to generate dialect specific attributes from ODS instead. Or perhaps being able to write some function manually would be useful.

Currently I generate wrapper types around Operation that provide additional methods, for example:

        pub struct AddIOp<'c> {
            operation: ::melior::ir::operation::Operation<'c>,
        }
        impl<'c> AddIOp<'c> {
            pub fn name() -> &'static str {
                "arith.addi"
            }
            pub fn operation(&self) -> &::melior::ir::operation::Operation<'c> {
                &self.operation
            }
            pub fn builder(
                location: ::melior::ir::Location<'c>,
            ) -> AddIOpBuilder<'c, AddIOp__No__Lhs, AddIOp__No__Rhs> {
                AddIOpBuilder::new(location)
            }
            pub fn result(&self) -> ::melior::ir::operation::OperationResult<'c, '_> {
                self.operation.result(0usize).expect("operation should have this result")
            }
            pub fn lhs(&self) -> ::melior::ir::Value<'c, '_> {
                self.operation
                    .operand(0usize)
                    .expect("operation should have this operand")
            }
            pub fn rhs(&self) -> ::melior::ir::Value<'c, '_> {
                self.operation
                    .operand(1usize)
                    .expect("operation should have this operand")
            }
        }

I then provide implementations of Into<Operation> and TryFrom<Operation> to "cast" to and from an Operation.

        impl<'c> TryFrom<::melior::ir::operation::Operation<'c>> for AddIOp<'c> {
            type Error = ::melior::Error;
            fn try_from(
                operation: ::melior::ir::operation::Operation<'c>,
            ) -> Result<Self, Self::Error> {
                Ok(Self { operation })
            }
        }
        impl<'c> Into<::melior::ir::operation::Operation<'c>> for AddIOp<'c> {
            fn into(self) -> ::melior::ir::operation::Operation<'c> {
                self.operation
            }
        }

I wonder if it would be better to use an OperationLike trait, similar to AttributeLike? That way we wouldn't have to call operation or into to use the methods on Operation.

On a related note: should we also generate Ref (and RefMut?) types for these operation wrappers? It might be useful to be able to cast a OperationRef into a AddIOpRef, for example, to more easily analyze operations in external passes (or even external analyses, which is something else I'm working on as well: mlir-rust-tools).

raviqqe · 2023-08-16T03:14:15Z

Thank you so much for the huge work! It's gonna take a few days to review it probably. So please wait patiently. 😄

raviqqe · 2023-08-16T07:24:21Z

I wanted complete parity with the existing hand-written dialect bindings, but there are some things that aren't generated as nicely. For example, arith::CmpiPredicate is not generated and plain Attribute is used instead for arith::cmpi

This is fine for now. What we should do first is to make this implementation of dialects usable. We can deal with the issues later.

raviqqe · 2023-08-16T07:27:01Z

I wonder if it would be better to use an OperationLike trait, similar to AttributeLike? That way we wouldn't have to call operation or into to use the methods on Operation.

Yeah, I think that would be a better solution. But it doesn't have to be a scope of this PR.

raviqqe · 2023-08-16T07:36:05Z

On a related note: should we also generate Ref (and RefMut?) types for these operation wrappers? It might be useful to be able to cast a OperationRef into a AddIOpRef, for example, to more easily analyze operations in external passes (or even external analyses, which is something else I'm working on as well: mlir-rust-tools).

If you think they are useful, we definitely can! Actually, I have little experience in modifying operations built already. For the RefMut stuff, as described in #24, I haven't decided yet on the best solution (the list item of "Mutable operations vs dynamic checks with RefCell (vs silently potentially unsafe mutability).") So we can use Ref for now.

raviqqe · 2023-08-16T07:38:56Z

It's already a pretty big PR. We can merge this one first, fix the build, tests, and CI, feature-flag these auto-generated dialects, and then improve the codes and features gradually. What do you think?

Danacus · 2023-08-16T07:58:42Z

On a related note: should we also generate Ref (and RefMut?) types for these operation wrappers? It might be useful to be able to cast a OperationRef into a AddIOpRef, for example, to more easily analyze operations in external passes (or even external analyses, which is something else I'm working on as well: mlir-rust-tools).

If you think they are useful, we definitely can! Actually, I have little experience in modifying operations built already. For the RefMut stuff, as described in #24, I haven't decided yet on the best solution (the list item of "Mutable operations vs dynamic checks with RefCell (vs silently potentially unsafe mutability).") So we can use Ref for now.

I can't say I have much experience with modifying MLIR operation either, I have mostly been hacking the LLVM backend, and MLIR just caught my interest. Now I'm wondering how and if similar things could be achieved in MLIR, and if I can use Rust for this. But some of the things I'm trying to achieve are likely out-of-scope for the MLIR C API and/or melior.

I also don't really have enough experience with Rust and ffi to know how to properly deal with mutability accross language boundaries (e.g. what if C++ code in MLIR is also holding a mutable pointer? is are mutable reference no longer valid then? can you ever be sure?).

It's already a pretty big PR. We can merge this one first, fix the build, tests, and CI, feature-flag these auto-generated dialects, and then improve the codes and features gradually. What do you think?

I think that's a great idea! That way PR's can have a more limited scope, making them easier to review and polish. I kind of feel bad for making such a large PR. Once I get started on something, I can't seem to stop myself 😅 Feel free to tell me when a PR is too big, or when it is not written well enough, or if there is any other reason you would prefer to not merge something. I don't mean to pressure you in any way.

Danacus · 2023-08-16T11:51:35Z

CI is failing because the documentation of dialect operations is generated from the TableGen files, which may contain some code blocks. Those code blocks are assumed to be valid rust code, although they do not contain rust code at all. There doesn't seem to be a nice way to deal with this right now (see rust-lang/rust#59867). The only solutions I can find are:

Hack the description strings to replace ``` with ```ignore
Disable the module in doctests using #[cfg(not(doctest))], but then any example in docs that references the dialect module will also fail to build.
Disable all doctests, but that decreases test coverage
Remove the autogenerated docs for now and solve this problem in a later PR.

macro/Cargo.toml

raviqqe · 2023-08-16T04:34:22Z

macro/src/dialect/mod.rs

+
+// Writes `tablegen_compile_commands.yaml` for any TableGen file that is being parsed.
+// See: https://mlir.llvm.org/docs/Tools/MLIRLSP/#tablegen-lsp-language-server--tblgen-lsp-server
+fn emit_tablegen_compile_commands(td_file: &str, includes: &[String]) {


I'm not sure if I understand why this is built here. I don't see any .td files committed or generated. How do you use the database file in your editor? Do you view the LLVM install directory from the crate's top directory?

You should probably remove it. I was creating some TableGen files in a separate directory and including them using the dialect! macro, but it doesn't really belong here.

raviqqe · 2023-08-16T04:44:51Z

macro/src/dialect/types.rs

+    def: Record<'a>,
+}
+
+#[allow(unused)]


How are you expecting this to be used?

These structs are used, there are just some functions that on these structs that aren't used. The attribute should be moved down, or these unused functions should be removed. Most of these structs are partial reimplementations of some classes in MLIR (e.g. https://mlir.llvm.org/doxygen/classmlir_1_1tblgen_1_1TypeConstraint.html), since it was easier to reimplement parts than to create bindings.

The operands and results in ODS operations are TypeConstraints, and attributes are AttributeConstraints.

macro/src/dialect/error.rs

raviqqe · 2023-08-16T07:22:49Z

macro/src/dialect/types.rs

+#[derive(Debug, Clone, Copy)]
+pub struct TypeConstraint<'a>(Record<'a>);
+
+#[allow(unused)]


These contraints too.

raviqqe · 2023-08-17T02:45:41Z

Are there any reasons you used panics rather than results in the codes especially in the macro crate?

raviqqe · 2023-08-17T02:55:28Z

macro/src/dialect/error.rs

+    Syn(syn::Error),
+    TableGen(tblgen::Error),
+    ExpectedSuperClass(SourceError<ExpectedSuperClassError>),
+    ParseError,


Why don't we directly propagate the errors from the tblgen crate?

raviqqe · 2023-08-17T03:03:11Z

macro/src/dialect/error.rs

+    }
+}
+
+impl From<Error> for syn::Error {


What is this implementation for?

I think it's a leftover from when the code was in a separate crate. I used to convert all errors to syn::Error and used syn::Error::into_compile_error. I guess this also why I made a custom error type, but it can probably be removed now.

Danacus · 2023-08-17T05:35:21Z

Are there any reasons you used panics rather than results in the codes especially in the macro crate?

Do you mean in the macro code itself, or in the generated code. If you mean the macro code itself, those should probably be replaced with results. My code initially didn't use results, but there are still some unwraps left that should be removed.

For the generated code, I'm assuming that each "typed" operation from a dialect is valid according to the dialect spec. I was thinking about verifying this in the TryInto implementations. However, it would still be possible that these operations are modified after they are verified such that they are no longer valid according to the dialect, in which case some of those accessor functions might panic, which isn't ideal.

Danacus · 2023-08-17T05:36:25Z

Thank you for reviewing this PR!

raviqqe · 2023-08-17T08:13:25Z

Hack the description strings to replace with ignore

I've marked all the code blocks with empty info strings text. Now, build, tests, and linting are all enabled on the CI with the ods-dialects feature flag.

Danacus · 2023-08-17T09:00:11Z

I've marked all the code blocks with empty info strings text. Now, build, tests, and linting are all enabled on the CI with the ods-dialects feature flag.

I had something similar in mind, but I forgot regex was a thing. I'm a little worried that there might be some non-markdown descriptions in some dialects, since I don't know how strict they are about this, but I guess it works for now.

For the RefMut stuff, as described in #24, I haven't decided yet on the best solution (the list item of "Mutable operations vs dynamic checks with RefCell (vs silently potentially unsafe mutability).") So we can use Ref for now.

I've been thinking about this, and I think I understand the problem now. When borrowing a Block for append_operation, ownership of the operation is moved into the block and a reference is returned, but this reference is tied to the borrow of the Block, so Block remains borrowed for as long as the operation reference exists.

If you would borrow Block mutably, the returned reference to the operation would be tied to the mutable borrow of block, meaning that we cannot access the block while the reference to the operation is live, which would be highly inconvenient.

Borrowing immutably from Block is slightly unsafe, since you do mutate the block to append the operation.

I'll think a bit about this issue, because I find it interesting, but I don't think I'll be able to come up with a better solution than anything you have in mind.

(I'm sorry if this is a bit off-topic)

raviqqe · 2023-08-17T09:07:04Z

Do you mean in the macro code itself, or in the generated code. If you mean the macro code itself, those should probably be replaced with results. My code initially didn't use results, but there are still some unwraps left that should be removed.

For the generated code, I'm assuming that each "typed" operation from a dialect is valid according to the dialect spec. I was thinking about verifying this in the TryInto implementations. However, it would still be possible that these operations are modified after they are verified such that they are no longer valid according to the dialect, in which case some of those accessor functions might panic, which isn't ideal.

I see. In that case, I think we should prefer Results in both macros and "typed" operations. For the "typed" operations, we can also restrict the conversion from "typed" operations to generic operations by moving them like:

impl From<FooOperation> for Operation { 
  fn from(operation: FooOperation) -> Operation {
    // ...
  }
}

But I'm not sure if it's possible yet.

Danacus · 2023-08-17T09:26:26Z

macro/src/dialect/operation/mod.rs

+            }
+
+            impl<'c> Into<::melior::ir::operation::Operation<'c>> for #class_name<'c> {
+                fn into(self) -> ::melior::ir::operation::Operation<'c> {


We already have impl From<FooOperation> for Operation through this impl of Into. I think From is preferred over Into though, but I'm not sure if we can implement From for melior::ir::Operation in foreign crates that might use this dialect! macro as well, which is why I chose to implement Into.

Apparently I was wrong about Into providing From, it's only the other way around. And I just read orphaning rules have become more relaxed, and you can implement From on foreign types now. This should be replaced with an impl of From, my bad!

As previously suggested in #274, I replaced the `.expect` with returning a `Result` in the operation accessors of ODS generated dialects. I also added some variants to the `Error` enum to support this. The `Infallible` variant was added to allow using `TryInto` to convert `Attribute` into itself, such that we avoid needing to handle `Attribute` differently from `StringAttribute`, `IntAttribute`, etc. But if you prefer, I can change the macro code to deal with this case instead, I'm honestly not a big fan of my `Infallible` hack either. --------- Co-authored-by: Yota Toyama <[email protected]>

Danacus added 2 commits August 14, 2023 15:26

Dialect generation

ef92c21

Clippy fixes and refactor

2274842

Add feature flag for dialects generated from ODS

19c7c05

Danacus mentioned this pull request Aug 16, 2023

Call to enable_result_type_inference is not safe #275

Closed

Add tests for (variadic) operands and regions

bc2ba6e

raviqqe reviewed Aug 17, 2023

View reviewed changes

raviqqe merged commit f9eece9 into mlir-rs:main Aug 17, 2023

raviqqe reviewed Aug 17, 2023

View reviewed changes

raviqqe mentioned this pull request Aug 17, 2023

Generation of dialect bindings with TableGen and proc macro #262

Closed

7 tasks

Danacus commented Aug 17, 2023

View reviewed changes

Danacus mentioned this pull request Aug 17, 2023

Return Result in accessors instead of panic #286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Dialect generation from ODS #274

Draft: Dialect generation from ODS #274

Danacus commented Aug 14, 2023 •

edited by raviqqe

Loading

raviqqe commented Aug 16, 2023

raviqqe commented Aug 16, 2023

raviqqe commented Aug 16, 2023

raviqqe commented Aug 16, 2023

raviqqe commented Aug 16, 2023

Danacus commented Aug 16, 2023

Danacus commented Aug 16, 2023

raviqqe Aug 16, 2023

Danacus Aug 17, 2023

raviqqe Aug 16, 2023

Danacus Aug 17, 2023

raviqqe Aug 16, 2023

raviqqe commented Aug 17, 2023 •

edited

Loading

raviqqe Aug 17, 2023

raviqqe Aug 17, 2023

Danacus Aug 17, 2023

Danacus commented Aug 17, 2023

Danacus commented Aug 17, 2023

raviqqe commented Aug 17, 2023 •

edited

Loading

Danacus commented Aug 17, 2023

raviqqe commented Aug 17, 2023 •

edited

Loading

Danacus Aug 17, 2023

Danacus Aug 17, 2023

Draft: Dialect generation from ODS #274

Draft: Dialect generation from ODS #274

Conversation

Danacus commented Aug 14, 2023 • edited by raviqqe Loading

raviqqe commented Aug 16, 2023

raviqqe commented Aug 16, 2023

raviqqe commented Aug 16, 2023

raviqqe commented Aug 16, 2023

raviqqe commented Aug 16, 2023

Danacus commented Aug 16, 2023

Danacus commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raviqqe commented Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danacus commented Aug 17, 2023

Danacus commented Aug 17, 2023

raviqqe commented Aug 17, 2023 • edited Loading

Danacus commented Aug 17, 2023

raviqqe commented Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danacus commented Aug 14, 2023 •

edited by raviqqe

Loading

raviqqe commented Aug 17, 2023 •

edited

Loading

raviqqe commented Aug 17, 2023 •

edited

Loading

raviqqe commented Aug 17, 2023 •

edited

Loading