This is an archive of the discontinued LLVM Phabricator instance.

[mlir][irdl] Add IRDL registration
ClosedPublic

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

Details

Summary

This patch add support for loading IRDL dialects at runtime
with mlir-opt.

Given the following dialect.irdl file:

mlir
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)
    }
}

the IRDL file can be loaded with the mlir-opt --irdl-file=dialect.irdl
command, and the following file can then be parsed:

mlir
func.func @conorm(%p: !cmath.complex<f32>, %q: !cmath.complex<f32>) -> f32 {
  %norm_p = "cmath.norm"(%p) : (!cmath.complex<f32>) -> f32
  %norm_q = "cmath.norm"(%q) : (!cmath.complex<f32>) -> f32
  %pq = arith.mulf %norm_p, %norm_q : f32
  return %pq : f32
}

To minimize the size of this patch, the operation, attribute, and type verifier are all always returning success().

Depends on D144692

Diff Detail

Event Timeline

math-fehr created this revision.Feb 23 2023, 6:36 PM
math-fehr requested review of this revision.Feb 23 2023, 6:36 PM
Mogball requested changes to this revision.Mar 6 2023, 11:30 AM
Mogball added inline comments.
mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
116

please spell out this auto

118

please emit this as an error through the MLIR context, using an UnknownLoc

135
165

please use empty

Mogball added inline comments.Mar 6 2023, 11:30 AM
mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
140

why is this here?

This revision now requires changes to proceed.Mar 6 2023, 11:30 AM
rriddle requested changes to this revision.Mar 6 2023, 11:33 AM

This patch seems to be doing a lot of things at once. I would split out registration in mlir-opt from the various IRDL things being added, and make things a bit more incremental, given the IRDL constructs could be tested via a test-pass (for example).

Mogball added inline comments.Mar 6 2023, 11:34 AM
mlir/include/mlir/Dialect/IRDL/IRDLVerifiers.h
38

please put public first and then private class members

77

same here

mlir/lib/Dialect/IRDL/IRDLRegistration.cpp
30

please use static instead of placing file-scope functions in an anonymous namespace

36

why append instead of <<?

44
56

we typically prefer unsigned to size_t

59

please spell out this auto, auto should only be used if the type is present verbatim on the right hand side of the assignment. Please audit all your autos and apply this change

mlir/lib/Dialect/IRDL/IRDLVerifiers.cpp
28

function_ref can be null

rriddle added inline comments.Mar 6 2023, 11:35 AM
mlir/lib/Dialect/IRDL/IR/IRDLOps.cpp
26

Prefer std::optional over llvm::Optional.

39

Can you use enumerate here instead?

mlir/lib/Dialect/IRDL/IRDLRegistration.cpp
26

Use static instead of anonymous namespaces for functions.

424

Use namespace qualifiers instead of wrapping namespaces in .cpp files: https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions

Same throughout the patch.

This patch seems to be doing a lot of things at once. I would split out registration in mlir-opt from the various IRDL things being added, and make things a bit more incremental, given the IRDL constructs could be tested via a test-pass (for example).

Do you mean removing the MLIROptMain change for another patch, and then test the rest of the patch in a test-pass (I guess I can add features one by one, like first registration of types/attributes, then of operations).

If I'm correct, you are proposing to have a pass similar to the PDL one that register rewrites that are part of the file (with the -test-pdl-bytecode-pass pass). However, I couldn't do something similar with IRDL, since we cannot parse the entire file until the attributes are registered from what I understand.

Is that what you meant? I probably misunderstood something

Mogball added a comment.EditedMar 6 2023, 11:43 PM

This patch seems to be doing a lot of things at once. I would split out registration in mlir-opt from the various IRDL things being added, and make things a bit more incremental, given the IRDL constructs could be tested via a test-pass (for example).

Do you mean removing the MLIROptMain change for another patch, and then test the rest of the patch in a test-pass (I guess I can add features one by one, like first registration of types/attributes, then of operations).

If I'm correct, you are proposing to have a pass similar to the PDL one that register rewrites that are part of the file (with the -test-pdl-bytecode-pass pass). However, I couldn't do something similar with IRDL, since we cannot parse the entire file until the attributes are registered from what I understand.

Is that what you meant? I probably misunderstood something

I think you should find a logical way to split this patch up in general. One way would be to do the MlirOptMain registration in a separate patch and slowly land the interfaces/verification stuff. At least, I think the op verification can be tested separately (unless the dynamic dialect has to be registered before the ops are loaded?)

I think you should find a logical way to split this patch up in general. One way would be to do the MlirOptMain registration in a separate patch and slowly land the interfaces/verification stuff. At least, I think the op verification can be tested separately (unless the dynamic dialect has to be registered before the ops are loaded?)

Is it possible to register new dialects during a pass?
Alternatively, what I can do is first add the MlirOptMain registration, which would register only the dialect, and then add type/attributes, then operations. That way, we still keep the same workflow, we just add features one by one.
Would that work for you?

Also, do you want me to split further D144692 as well?

IMO that would make sense to me. Anything to land these changes incrementally.

Thanks! I'll fix these issues, and split that patch to smaller pieces, to make this more incremental. I'll do this first thing tomorrow!

math-fehr updated this revision to Diff 503451.Mar 8 2023, 11:11 AM
math-fehr marked 17 inline comments as done.

Address comments, and remove AttributeWrapper

I addressed all the code style comments, and move some code away.
In order to make this increasingly incremental, I'll move out completely the any_of operation from irdl, which should remove quite a lot of code.

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

I should have changed all the ones in my code, besides these two exceptions:

  • Lambdas, since their operands are spelled out anyway
  • for (auto [i, value] : enumerate(myIterator))

Is that correct, or should these as well be spelled outs?

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
140

From my understanding, multithreading needs to be disabled when we register dialects.
I added a better comment at the location where we disable multithreading.
Otherwise, I can only inject the thread pool after the registerIRDL call, if you think this is better?

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

Remove any_of from irdl

math-fehr planned changes to this revision.Mar 8 2023, 1:08 PM

I removed any_of from irdl, to simplify the registration.
I still plan on simplifying this patch further though, and split it up in more pieces.

math-fehr updated this revision to Diff 503521.Mar 8 2023, 2:23 PM

Remove verification of constraints

This should make the patch way smaller.
Now, while operations, types, and attributes are registered, they
are unconstrained, so they will always verify.

I'll add the constraints on a separate patch.

The patch should be way smaller now, I removed the verification logic, and will add it in a subsequent patch.

math-fehr edited the summary of this revision. (Show Details)Mar 9 2023, 3:27 PM

Is it possible to register new dialects during a pass?

No it isn't. We should runtime_error on this.

Is it possible to register new dialects during a pass?

No it isn't. We should runtime_error on this.

Is there a way to add a check for this? Currently, I'm removing the multi-threading, but I guess that you would like a concrete check?

Is there a way to add a check for this?

The check exists for non IRDL cases here: https://github.com/llvm/llvm-project/blob/9e0c2626f3d4282a135da112320139d9fba32fb5/mlir/lib/IR/MLIRContext.cpp#L444-L451
(despite the name, it'll kick in even if one disables multi-threading)

but I guess that you would like a concrete check?

Yes :)

mlir/include/mlir/Dialect/IRDL/IRDLRegistration.h
24

I feel like you're using "register" when it is actually more "loading" here.
(I'm not sure IRDL has a concept equivalent to "registering" dialects?)

mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h
64

This method is deprecated and will be removed, can you rebase and use the MlirOptMainConfig instead?

mlir/lib/Dialect/IRDL/IR/IRDLOps.cpp
13

All these includes don't seem to belong to this patch anymore.

mehdi_amini added inline comments.Mar 9 2023, 3:57 PM
mlir/lib/Dialect/IRDL/IRDLRegistration.cpp
56

I strictly use signed int :)

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
140

You don't need to touch multi-threading to register/load dialects. These just aren't thread-safe APIs and shouldn't be used during PassManager execution

math-fehr added inline comments.Mar 9 2023, 4:03 PM
mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
140

This is done during MlirOptMain though, before the PassManager is setup (from my understanding).
I recall having some non-deterministic bugs at some points, and disabling multithreading at the registration time was the solution.

So should this pass even with multithreading? Because what this function does is both a registration and a loading.

mehdi_amini added inline comments.Mar 9 2023, 4:22 PM
mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
140

Multi-threading isn't a factor here: even if the thread pool has been created, there are no active threads in the context.
So unless your process is involving thread that's just fine.

math-fehr updated this revision to Diff 504285.Mar 10 2023, 2:29 PM
math-fehr marked 6 inline comments as done.

Address additional comments

I feel like you're using "register" when it is actually more "loading" here. (I'm not sure IRDL has a concept equivalent to "registering" dialects?)

Coming back to this: how do we differentiate registering from loading in IRDL? Do you see some difference somehow?

I feel like you're using "register" when it is actually more "loading" here. (I'm not sure IRDL has a concept equivalent to "registering" dialects?)

Coming back to this: how do we differentiate registering from loading in IRDL? Do you see some difference somehow?

In IRDL, there are not really any registration, since the dialects are loaded right away, without going through the `DialectRegistry.
While there may be a way to allow registering without loading, I'm not completely sure what would be the actual benefits, and I'm not sure how easy this would be.

The way we *could* make this work with an actual benefit would be to keep the IRDL program in memory for the entire compilation process,
and to register our dialects in the DialectRegistry with just a pointer to the corresponding DialectOp. Then, we would actually do the
loading and the main registration logic only if the dialect is required.
This could work, though I didn't work on it since I'm not sure what would be the advantages yet.

mlir/include/mlir/Dialect/IRDL/IRDLRegistration.h
24

True, I changed it to "load'.
However, this is something I didn't really think about when writing the dynamic dialects,
so the dynamic dialects are still using the "register" terminology.

mlir/lib/Dialect/IRDL/IRDLRegistration.cpp
56

I moved them to unsigned in the meantime, what would be the best fit then?

I feel like you're using "register" when it is actually more "loading" here. (I'm not sure IRDL has a concept equivalent to "registering" dialects?)

Coming back to this: how do we differentiate registering from loading in IRDL? Do you see some difference somehow?

In IRDL, there are not really any registration, since the dialects are loaded right away, without going through the `DialectRegistry.
While there may be a way to allow registering without loading, I'm not completely sure what would be the actual benefits, and I'm not sure how easy this would be.

The main issue I was trying to solve when designing this in MLIR was to preserve consistency of the system: we can't run a pipeline that accidentally crash because a dialect wasn't loaded in the first place for example: passes will declare dialects they need as "dependent". We also didn't want to always load all dialects preemptively if not needed, and in particular to be allow to test everything (including the "getDependentDialect" for passes) we needed mlir-opt to not load any dialect that isn't in the input IR.

Now, I don't know how IRDL plays with passes actually? What is the kind of use-cases we'll have for IRDL in mlir-opt?

mlir/lib/Dialect/IRDL/IRDLRegistration.cpp
56

This is fine, you do as you prefer.

There is no rules in LLVM, so people like me will always used signed and others will prefer unsigned. So unfortunately we're inconsistent!

FYI, see my previous attempt to define a standard: https://discourse.llvm.org/t/rfc-coding-standards-prefer-int-for-regular-arithmetic-use-unsigned-only-for-bitmask-and-when-you-intend-to-rely-on-wrapping-behavior/52191

I feel like you're using "register" when it is actually more "loading" here. (I'm not sure IRDL has a concept equivalent to "registering" dialects?)

Coming back to this: how do we differentiate registering from loading in IRDL? Do you see some difference somehow?

In IRDL, there are not really any registration, since the dialects are loaded right away, without going through the `DialectRegistry.
While there may be a way to allow registering without loading, I'm not completely sure what would be the actual benefits, and I'm not sure how easy this would be.

The way we *could* make this work with an actual benefit would be to keep the IRDL program in memory for the entire compilation process,
and to register our dialects in the DialectRegistry with just a pointer to the corresponding DialectOp. Then, we would actually do the
loading and the main registration logic only if the dialect is required.
This could work, though I didn't work on it since I'm not sure what would be the advantages yet.

I feel like you're using "register" when it is actually more "loading" here. (I'm not sure IRDL has a concept equivalent to "registering" dialects?)

Coming back to this: how do we differentiate registering from loading in IRDL? Do you see some difference somehow?

In IRDL, there are not really any registration, since the dialects are loaded right away, without going through the `DialectRegistry.
While there may be a way to allow registering without loading, I'm not completely sure what would be the actual benefits, and I'm not sure how easy this would be.

The main issue I was trying to solve when designing this in MLIR was to preserve consistency of the system: we can't run a pipeline that accidentally crash because a dialect wasn't loaded in the first place for example: passes will declare dialects they need as "dependent". We also didn't want to always load all dialects preemptively if not needed, and in particular to be allow to test everything (including the "getDependentDialect" for passes) we needed mlir-opt to not load any dialect that isn't in the input IR.

Now, I don't know how IRDL plays with passes actually? What is the kind of use-cases we'll have for IRDL in mlir-opt?

My feeling is that we explicitly do not want a pass to depend on a specific IRDL dialect. In fact, this is already something that we do not want to have for DynamicDialect.
Having a pass relying on a dynamic dialect would be too dangerous and error-prone, and if you are already paying the cost of recompiling to write your pass, you are willing to pay the cost of compiling your dialect to C++ (in my opinion).
Also, manipulating IRDL dialects in C++ is a bit annoying, since you need to use the generic way of accessing operands/results/attributes, meaning that you have almost no guarantee that those operands/results/attributes exist in the first place.

However, having passes that use Traits or Interfaces to manipulate these dialects is something that is useful, though we would need to first add Traits and Interfaces to IRDL, which may be complex in some cases (since some heavily rely on C++ templates).

That being said, our objective with having IRDL in mlir-opt is to then be able to load PDL rewrites dynamically as well, to have a completely dynamic pipeline. Probably the transform dialect could be useful there as well.
This will of course require more work to make the interface nice enough for a user of mlir-opt.

My feeling is that we explicitly do not want a pass to depend on a specific IRDL dialect. In fact, this is already something that we do not want to have for DynamicDialect.
Having a pass relying on a dynamic dialect would be too dangerous and error-prone, and if you are already paying the cost of recompiling to write your pass, you are willing to pay the cost of compiling your dialect to C++ (in my opinion).
Also, manipulating IRDL dialects in C++ is a bit annoying, since you need to use the generic way of accessing operands/results/attributes, meaning that you have almost no guarantee that those operands/results/attributes exist in the first place.

Sure, but that means that no pass will every create entities (Operation/Type/Attribute) defined by an IRDL dialect? If you think otherwise: how would that happen?

That being said, our objective with having IRDL in mlir-opt is to then be able to load PDL rewrites dynamically as well, to have a completely dynamic pipeline. Probably the transform dialect could be useful there as well.
This will of course require more work to make the interface nice enough for a user of mlir-opt.

I'd have to think more about the whole consistency of the system, but we can punt on this for now :)

mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h
32
mehdi_amini accepted this revision.Mar 12 2023, 6:33 AM

This LGTM, but @rriddle and @Mogball had concerned earlier, it would be great if they could take another look here!

Sure, but that means that no pass will every create entities (Operation/Type/Attribute) defined by an IRDL dialect? If you think otherwise: how would that happen?

For now, I'm fine with this, since there are currently no big benefit on defining a dialect in IRDL if you want to write a C++ pass.
However, I agree that in the future, we might want to lift this in order to allow passes to create entities defined in IRDL, especially once we manage to get traits and interfaces in IRDL.

From my understanding though, mlir::Pass require to specify the dependent dialects as class names. This would probably need to be changed (or we would need a separate function),
if we would like to support this for IRDL. This is because we do not have a class for IRDL dialects, so we cannot just insert the dialects in the registry without knowing in which file
the dialects are defined.

That being said, our objective with having IRDL in mlir-opt is to then be able to load PDL rewrites dynamically as well, to have a completely dynamic pipeline. Probably the transform dialect could be useful there as well.
This will of course require more work to make the interface nice enough for a user of mlir-opt.

I'd have to think more about the whole consistency of the system, but we can punt on this for now :)

math-fehr updated this revision to Diff 504477.Mar 12 2023, 3:01 PM

Update comment (register -> load)

math-fehr marked an inline comment as done.Mar 12 2023, 3:01 PM
rriddle accepted this revision.Apr 2 2023, 10:59 PM

Looks like a good start to me

mlir/include/mlir/Dialect/IRDL/IRDLLoading.h
16–17 ↗(On Diff #504477)

Can you use forward declarations here instead?

mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h
37

Is the llvm:: here and above necessary?

mlir/lib/Dialect/IRDL/IRDLLoading.cpp
49–50 ↗(On Diff #504477)

Is this comment wrapped properly at 80? It looks prematurely split.

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
133–135
166–169
math-fehr updated this revision to Diff 510633.Apr 3 2023, 4:03 PM
math-fehr marked 5 inline comments as done.

Address comments

Mogball accepted this revision.Apr 14 2023, 9:34 AM

sorry. Forgot I was blocking on this. LGTM

This revision is now accepted and ready to land.Apr 14 2023, 9:34 AM
This revision was automatically updated to reflect the committed changes.
math-fehr reopened this revision.Apr 23 2023, 7:42 AM
This revision is now accepted and ready to land.Apr 23 2023, 7:42 AM
math-fehr updated this revision to Diff 516166.Apr 23 2023, 7:43 AM

Rebase to latests LLVM, and attempt to fix compilation

This revision was automatically updated to reflect the committed changes.