This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Introduce IRDL dialect
ClosedPublic

Authored by math-fehr on Feb 23 2023, 6:17 PM.

Details

Summary

This patch introduces the IRDL dialect, which allow users to represent
dynamic dialect definitions as an MLIR program.

The IRDL dialect defines operations, attributes, and types, using
attribute constraints. For example:

module {
  irdl.dialect @cmath {
    irdl.type @complex {
      %0 = irdl.is f32
      %1 = irdl.is f64
      %2 = irdl.any_of(%0, %1)
      irdl.parameters(%2)
    }

    irdl.operation @norm {
      %0 = irdl.any
      %1 = irdl.parametric @complex<%0>
      irdl.operands(%1)
      irdl.results(%0)
    }
}

This program will define a new cmath.complex type, which expects a single
parameter, which is either an f32 or an f64. It also defines an
cmath.norm operation, which expects a single cmath.complex type as operand,
and returns a value of the underlying type. Note that like PDL (which IRDL is
heavily inspired from), both uses of %0 are expected to be of the same attribute.

IRDL handles attributes and types with the same operations, and does this by always
wrapping types in a TypeAttr. This is to simplify the language.

Depends on D144690

Diff Detail

Event Timeline

math-fehr created this revision.Feb 23 2023, 6:17 PM
math-fehr requested review of this revision.Feb 23 2023, 6:17 PM

Nice work! I have some comments inline, mostly to understand the dialect.

mlir/include/mlir/Dialect/IRDL/IR/IRDLOps.td
144

Many patterns have matches against any type or any shaped type, etc. I'm guessing you'd have to create a type that means any type, but not necessarily any attribute, so irdl.any doesn't cut. Attributes are also "open world" (unbound list), so better to add all types than to restrict attributes.

Perhaps a "library" of those things (always included) would do the trick, where you define irdl.any_int to be a union of all known integers, irdl.any_float with floats, etc. and irdl.any_type to be the union of all irdl.any_*_type.

156

%2 = any_of missing here

161

Some of your examples use parameters(%2), others parameters(%0, %0) for the same type declaration (complex). It's unclear to me how to use this.

It could be that parameter(%2) means a restriction for any type in a complex type, and that parameter(%0, %1) could have different types for complex, but they'd have to be declared differently, ex. complex<%0> vs. complex<%1, %2>.

I find the latter way (explicit in the number of types) more appealing, since it gives me information on all of the types, even if they're the same.

Is that what it means? Or am I just mixing semantics?

220

I'm imagining to support variadic here you'd probably need to create a new operation (like irdl.var) which would have type irdl.any_type or something, and just lower to the Variadic argument with the right type.

229

High-level question: Will IRDL detect this case where all ops must have the same type and add the appropriate trait to the op declaration? Similar for other attributes like isolatedFromAbove etc.

From the RFC, this seems to be in the roadmap, I'm just curious as if the intention is to request users to add manually the constraints or to automatically detect at least some basic ones.

309

Right, we could use this to create the irdl.any_int classes.

Thanks! I'll add the any_type, and cleanup a bit some parts of the documentation!

mlir/include/mlir/Dialect/IRDL/IR/IRDLOps.td
144

Good point, I'll add irdl.any_type for this patch!

Otherwise, irdl.any_float should be exactly irdl.any_of.(%0, %1, %2, %3, ...), where the variables are here the different possible floats. Now maybe we should find a way to create aliases (maybe using functions?), so we could create a any_float function somewhere, that people could use with a irdl.call @any_float() (which would be inlined later). Though I believe we should put that in a different patch.

For any_int, this is a bit more complex, since we would need to create a wrapper for builtin.integer, but this is complex since bulitin.integer does not have attribute parameters. My solution for this would be to use custom constraints, which are constraints defined by the user in C++. Once I add them, I agree we should have some sort of "library" of the most common ones, like "any_integer". But I would also suggest to do this in a following patch, if that makes sense?

My overall feeling is that we should try to keep a small "core calculus" for IRDL, so we can easily interpret it. Then we add more complex features on top through aliases and custom constraints.

156

Thanks! I'll fix this.

161

My mistake, I'll change the example to make this clearer! I should not have used the same name twice!

parameters(%2) declares that the attribute/type has only a single parameter, while parameters(%0, %0) declares that the attribute has two parameters, that are exactly the same (in practice, no one should write this).

220

Variadics might be a little bit more complex, because we need to differentiate A variadic number of operands of type %0 and A variadic number of operands of type %0, that all have the same type.

My feeling is that this will be implemented using regions, something like:

%1 = irdl.variadic {
  %0 = irdl.any_type
  irdl.yield %0
}

to represent a variadic of heterogeneous types.

and

%0 = irdl.any_type
%1 = irdl.variadic {
  irdl.yield %0
}

to represent a variadic of homogeneous types.

The semantics would be that any variable inside an irdl.variadic is reset at each check inside the variadic.
But this is something we should probably explore more, to find a possible better design.

229

Since we can detect it, I see no reasons not to include that later! Note that the trait would be useful to interacting with the operation, but otherwise the verifier itself is already performing intelligent checks for that.

309

Yes, that's how I see this at least!

mostly stylistic comments

mlir/lib/Dialect/IRDL/AttributeWrapper.cpp
16 ↗(On Diff #500029)

please use isa<TypeAttr>(attr)

20 ↗(On Diff #500029)
26 ↗(On Diff #500029)

please drop the llvm:: prefix. They aren't needed

mlir/lib/Dialect/IRDL/IR/IRDL.cpp
64

empty?

mlir/lib/Dialect/IRDL/IR/IRDLAttributes.cpp
32 ↗(On Diff #500029)
math-fehr updated this revision to Diff 503074.Mar 7 2023, 9:43 AM
math-fehr marked 11 inline comments as done.

Address comments

I addressed the comments. I'm currently splitting this patch again, by removing support of TypeWrapper and AttributeWrapper.
This should make it easier to review, and I'll add support for these constructs in a follow-up patch.

mlir/include/mlir/Dialect/IRDL/IR/IRDLOps.td
144

Actually, if this is fine for you, I'd prefer adding it on a following patch.
Right now, this patch is already quite big, and I would prefer keep things relatively small if that makes sense?

math-fehr updated this revision to Diff 503140.Mar 7 2023, 1:16 PM

Remove attribute wrappers

I removed attribute wrappers from IRDL to make this patch much smaller. I'll add them back after the registration patches.

I think this patch should now be with minimal features, and only does the IRDL dialect definition.

rengolin added inline comments.Mar 8 2023, 12:09 PM
mlir/include/mlir/Dialect/IRDL/IR/IRDLOps.td
144

Absolutely! That's a big change indeed.

math-fehr updated this revision to Diff 503480.Mar 8 2023, 1:01 PM

Remove any_of from irdl

math-fehr marked an inline comment as done.Mar 8 2023, 1:03 PM

To further simplify this patch (and the registration one), I removed any_of from irdl.
I'll add it in the patch right after the registrations one.

However, I kept the mentions to any_of in the documentation, if that is fine for you, to not rewrite almost all the code snippets (since all of them used any_of).

To further simplify this patch (and the registration one), I removed any_of from irdl.
I'll add it in the patch right after the registrations one.

However, I kept the mentions to any_of in the documentation, if that is fine for you, to not rewrite almost all the code snippets (since all of them used any_of).

I don't mind a minimal version of any_of being here and then improved later. If you really want to split the work, then it would be good to see the patch series now, which is a bit more convoluted.

Basically, create another patch that depends on this one, and so forth until you can run a test that demonstrates a minimal functionality.

From what I read, you probably need about three patches to get to a nice place: This one, the registration one and the final tie on basic ops.

As it seems having types without any_of would be a bit pointless, not having it here would still need to show "elsewhere" how it would be implemented...

PS: I have just merged the dependency D144690.

As it seems having types without any_of would be a bit pointless, not having it here would still need to show "elsewhere" how it would be implemented...

That makes sense, I'm adding this in a follow-up patch.
The reason why I removed any_of in particular was that it requires non-trivial code in the registration, around
100+ lines of code to check that we won't have backtracking problems in the registration.

Currently, I want to have 5+ patches:

  • This one, that just introduce the dialect
  • D144692, which introduces the registration, by just registering operations and types without checking their correctness
  • 1 or more patches to introduce the actual verification logic. This change is still 600+ lines so I'm trying to make it into smaller patches
  • Introduction of any_of
  • Introduction of type and attribute wrappers, so we can use C++ types

That makes sense, I'm adding this in a follow-up patch.

Awesome, thanks!

Can you name the patches "[1/5] ...", "[2/5] ..." etc? Makes it easier to review when we know in which order they are. The parent/child relationship in Phab isn't super evident.

As with other patch series, we review all patches and only when all patches are approved you can merge them all at once. This is to make sure all points are covered, and to make it easier to pre-approve the good ones and focus discussion on the hard ones.

As with other patch series, we review all patches and only when all patches are approved you can merge them all at once. This is to make sure all points are covered, and to make it easier to pre-approve the good ones and focus discussion on the hard ones.

I don't believe there is any rule like that: I merge my patches as they get approved in general. We mostly need to agree about the overall design, then we can develop incrementally in tree. Stacked patches are mostly a convenient way of streamlining the development and review process.

That makes sense!
In that case I'll make it a 5 patch series that should include almost everything.
I'll keep a last patch for after this is merged, which is the AttributeWrapper stuff, that I want to rework a bit, and that is less important for now!

mehdi_amini added inline comments.Mar 9 2023, 3:44 PM
mlir/include/mlir/Dialect/IRDL/IR/IRDLTraits.h
25

Should make a note that this isn't recursive (you're not "walking" into the region)

38
47

If there are multiple ChildOp, this will traverse the region multiple times, can you look into having a single traversal?

75

(newline)

mlir/include/mlir/Dialect/IRDL/IR/IRDLTypes.td
46

Can you revisit this? At minima there is no %2 in the example.

I added all of the major patches (the following ones can be purely incremental)

  • D144692 [mlir] Introduce IRDL dialect: Defining the dialect itself
  • D144693 [mlir][irdl] Add IRDL registration: Register the dialects in mlir-opt, but without the verification logic
  • D145733 [mlir][irdl] Add IRDL verification constraint classes: Adding the verification infrastructure, without using it
  • D145734 [mlir][irdl] Add verification of IRDL ops: Use the verification infrastructure when registering IRDL dialects
  • D145735 [mlir][irdl] Add irdl.any_of operation: Add the last core operation, any_of

I don't believe there is any rule like that: I merge my patches as they get approved in general. We mostly need to agree about the overall design, then we can develop incrementally in tree. Stacked patches are mostly a convenient way of streamlining the development and review process.

Approving a patch-set in bulk isn't new or seldom used. We've been doing this in LLVM for at least a decade for all sorts of new features, including all new targets. What you describe is a completely different scenario.

We can't "agree about the overall design" if we can't see what the design is. That's the point of the initial patch-set being approved in bulk.

I'm not asking @math-fehr to post his entire work in one go, just those initial patches that would let us see "the overall design", but some parts may need more scrutiny than others.

After the initial set is approved and merged, then the work continues as usual, one patch at a time.

I don't believe there is any rule like that: I merge my patches as they get approved in general. We mostly need to agree about the overall design, then we can develop incrementally in tree. Stacked patches are mostly a convenient way of streamlining the development and review process.

Approving a patch-set in bulk isn't new or seldom used. We've been doing this in LLVM for at least a decade for all sorts of new features, including all new targets. What you describe is a completely different scenario.

I don’t think so: I am describing how most of MLIR has been developed and how most of my work in LLVM in general happened ( I think I have my fair share of contributing and reviewing new features in LLVM and MLIR to know what I am talking about?).
What I am pointing at here is that your experience isn’t necessarily THE way, and that there is a spectrum and a lot of variability, yet you seem to present it as such.

We can't "agree about the overall design" if we can't see what the design is.

There is much more than code ; for any large components we’ve been operating with open meeting presentation and RFC to understand designs. In this case there is even a paper describing the design. “Understanding the design” is important, it just does not necessarily imply to have all the code ready.

Many large MLIR components were landed that way.
Take the “Action” framework for example I am currently landing in the codebase, https://reviews.llvm.org/D144811 landed much before the entire stack was reviewed. For other components we started landing patches much earlier in the stack.

Anyway, let go back to the review and when this patch is approved we should just feel free to merge it.

math-fehr updated this revision to Diff 504139.Mar 10 2023, 7:19 AM
math-fehr marked 5 inline comments as done.

Address comments

Please tell me if the style of getChildOpIndex makes sense or
not, I have the feeling this could be rewritten in a nicer way,
but I have no clue how.

math-fehr updated this revision to Diff 504270.Mar 10 2023, 1:19 PM

Rebase to main

mehdi_amini added inline comments.Mar 10 2023, 1:59 PM
mlir/include/mlir/Dialect/IRDL/IR/IRDLTraits.h
41

I think we can avoid the test here by adding an operand:

/// Return the index of the operation concrete type in the variadic list of
/// operation concrete types.
/// Return -1 if the operation class is not in the list.
template <typename Op, typename... Ops>
static int getChildOpIndex(Operation *op, int index = 0) {
  if (llvm::isa<Op>(op))
    return index;

  return getChildOpIndex<Ops...>(op, index+1);
68

What provides the getOps() here? Is there a traits for that or something?
Probably something to add to the class documentation as well as a contract on the template argument.

mlir/include/mlir/Dialect/IRDL/IR/IRDLTypes.td
47

The documentation says that This type represents a handle to an instance of an mlir::Attribute, but %1 is more like a handle to an mlir::Type right?

math-fehr added inline comments.Mar 10 2023, 2:09 PM
mlir/include/mlir/Dialect/IRDL/IR/IRDLTraits.h
68

This comes from OneRegion, which is added in the static_assert, is that the right way to do this?

Otherwise, I'll add the documentation on the class!

mlir/include/mlir/Dialect/IRDL/IR/IRDLTypes.td
47

In the current design, everything in IRDL is represented as an mlir::Attribute.
Types are stored in a TypeAttr. This makes the language much simpler overall, especially since dynamic types and attributes have Attribute parameters.

math-fehr updated this revision to Diff 504283.Mar 10 2023, 2:27 PM
math-fehr marked an inline comment as done.

Address some comments

mehdi_amini added inline comments.Mar 10 2023, 3:10 PM
mlir/include/mlir/Dialect/IRDL/IR/IRDLTraits.h
68

That's fine, I forgot there is a getOps() expose by the Region class directly.

mlir/include/mlir/Dialect/IRDL/IR/IRDLTypes.td
47

That's kind of what I remembered, but I think we need to make a better effort on the documentation because we don't have a direct matching of mlir::Attribute and mlir::Type to direct concepts in IRDL.
The documentation here does not mention anything about types for example.

math-fehr updated this revision to Diff 504297.Mar 10 2023, 4:00 PM

Document the handling of mlir::Type in IRDL.

math-fehr marked 4 inline comments as done.Mar 10 2023, 4:01 PM
math-fehr added inline comments.
mlir/include/mlir/Dialect/IRDL/IR/IRDLTypes.td
47

I documented it in the dialect documentation, and the irdl.type documentation, tell me what you think, and if you think I should document it in some other places.

mehdi_amini accepted this revision.Mar 14 2023, 2:59 AM

This LGTM, but River may have thoughts on simplifying getChildOpIndex(), let's wait to hear back from him.

This revision is now accepted and ready to land.Mar 14 2023, 2:59 AM
rriddle added inline comments.Mar 15 2023, 12:02 AM
mlir/include/mlir/Dialect/IRDL/IR/IRDL.h
18–25

Can you trim some of these?

mlir/include/mlir/Dialect/IRDL/IR/IRDL.td
39

The names here are missing the symbol reference marker

mlir/include/mlir/Dialect/IRDL/IR/IRDLOps.td
42–43
134–137

Can we instead define a getParentOp helper on the HasParent trait when a single parent type is given?

276

For each of these, please use "" for single line strings.

284–285
371

Why is there a space here?

mlir/include/mlir/Dialect/IRDL/IR/IRDLTraits.h
38–39

Let's avoid recursion here, this should be viable to write a somewhat simple fold expression:

template <typename... Ops>
static int getChildOpIndex(Operation *op) {
  // A short-circuit fold that increments an index until we find a match. If
  // the fold results in true, nothing matched.
  int index = 0;
  if (((isa<Ops>(op) ? false : (++index, true)) && ...))
    return -1;
  return index;
}

Would also remove the other function specialization, and this function could then just be inlined into its use (which would also remove the -1 handling).

mlir/lib/Dialect/IRDL/IR/IRDL.cpp
59

Please drop the trivial braces.

67

Drop the trivial braces here.

math-fehr updated this revision to Diff 505679.Mar 15 2023, 6:30 PM
math-fehr marked 11 inline comments as done.

Address comments

math-fehr requested review of this revision.Mar 23 2023, 7:43 AM
rriddle accepted this revision.Apr 2 2023, 10:56 PM
This revision is now accepted and ready to land.Apr 2 2023, 10:56 PM
math-fehr updated this revision to Diff 510535.Apr 3 2023, 9:36 AM

Rebase to latest LLVM

Thank! Could you land this for me? I do not have commit access.

This revision was landed with ongoing or failed builds.Apr 3 2023, 10:08 AM
Closed by commit rG31229d48bbfd: [mlir] Introduce IRDL dialect (authored by math-fehr, committed by mehdi_amini). · Explain Why
This revision was automatically updated to reflect the committed changes.

I just got an email with two buildbot failures:
https://lab.llvm.org/buildbot/#/builders/181/builds/16214
https://lab.llvm.org/buildbot/#/builders/180/builds/14593

Should we revert the commit? I believe this is this patch breaking it?

@math-fehr Have you considered asking for commit access? It's not too difficult to request, and you've sent enough commits that it seems very reasonable.

Right, I'll try to ask for commit access, I wasn't sure what are the requirements for it!

Thanks for reverting it, I'm trying to reproduce it locally and find a fix!

math-fehr reopened this revision.Apr 3 2023, 2:40 PM
This revision is now accepted and ready to land.Apr 3 2023, 2:40 PM
math-fehr updated this revision to Diff 510612.Apr 3 2023, 2:40 PM

Fix MLIRIRDL CMake requirements

It should be good now, I forgot dependencies in the CMake!

This revision was automatically updated to reflect the committed changes.