Page MenuHomePhabricator

[MLIR] Introduce a trait that defines a new scope for auto allocation
ClosedPublic

Authored by bondhugula on Apr 9 2020, 3:15 AM.

Details

Summary

Introduce a new operation trait (AutomaticAllocationScope) for operations
with regions that define a new scope for automatic allocations;
such allocations (typically realized on stack) are automatically freed when
control leaves such ops' regions. std.alloca's are freed at the closest
surrounding op that has this trait. All FunctionLike operations should normally
have this trait.

Diff Detail

Event Timeline

bondhugula created this revision.Apr 9 2020, 3:15 AM
bondhugula retitled this revision from [MLIR] Introduce a trait for ops that define a new scope for auto allocation to [MLIR] Introduce a trait that defines a new scope for auto allocation.
ftynse added a comment.Apr 9 2020, 4:45 AM

Would it be possible to implement this without modifying Operation, i.e. as a simple trait rather than a trait based on a core operation property? Unlike "IsTerminator" and "IsolatedFromAbove", we don't have core mechanisms (which are the block verifier and the symbol resolver in the mentioned cases) that would mandate exposing this trait in Operation APIs.

mlir/include/mlir/Dialect/GPU/GPUOps.td
89

Nit: can we keep this sorted alphabetically?

Would it be possible to implement this without modifying Operation, i.e. as a simple trait rather than a trait based on a core operation property? Unlike "IsTerminator" and "IsolatedFromAbove", we don't have core mechanisms (which are the block verifier and the symbol resolver in the mentioned cases) that would mandate exposing this trait in Operation APIs.

The 'OperationProperty' enum class is missing its doc comment entirely - on how it's different from a trait that is not an OperationProperty. We should fix that first - the necessary information can't be determined locally without documentation.

ftynse added a comment.Apr 9 2020, 7:07 AM

Most traits are not an OperationProperty, it suffices to have a quick look in OpBase.td and see ~30 different traits being used. While we should add documentation to this enum (ping @rriddle), this is not precluding you from just defining a trait and landing it.

bondhugula added a comment.EditedApr 9 2020, 9:05 AM

Most traits are not an OperationProperty, it suffices to have a quick look in OpBase.td and see ~30 different traits being used. While we should add documentation to this enum

This justification doesn't make much sense, and can't be the basis to make a choice for a new trait. The traits are vastly different in what they mean.

(ping @rriddle), this is not precluding you from just defining a trait and landing it.

Just using a trait that doesn't use an OperationProperty bit is pretty straightforward, but simply landing the patch quickly isn't the point. What's more important here is to have the understanding and information documented on when something is a trait and an 'OperationProperty' as well - so that future contributors adding traits do the right thing. This should go into the doc comment on 'OperationProperty'. I'm assuming you aren't blocked by this revision.

bondhugula marked an inline comment as done.Apr 9 2020, 9:09 AM
bondhugula updated this revision to Diff 256323.Apr 9 2020, 9:09 AM

Address review comments.

bondhugula updated this revision to Diff 256324.Apr 9 2020, 9:11 AM

Sort trait list.

bondhugula edited the summary of this revision. (Show Details)Apr 9 2020, 9:11 AM
ftynse accepted this revision.Apr 9 2020, 9:21 AM

Just using a trait that doesn't use an OperationProperty bit is pretty straightforward, but simply landing the patch quickly isn't the point. What's more important here is to have the understanding and information documented on when something is a trait and an 'OperationProperty' as well - for the benefit of those adding traits in the future. This should go into the doc comment on 'OperationProperty'. I'm assuming you aren't blocked by this revision.

We are in a situation where we have upstream code for IR that has unclear/unspecified behavior, which was pointed out by reviewers. I consider this to be an issue similar to a new bug being introduced by a change, i.e. we may want to revert the offending change. The present diff specifies this behavior, so I consider it a fix to a problem upstream. As such, it should land once accepted instead being blocked on a missing comment, that has been missing for more than a year.

Harbormaster completed remote builds in B52526: Diff 256324.

Just using a trait that doesn't use an OperationProperty bit is pretty straightforward, but simply landing the patch quickly isn't the point. What's more important here is to have the understanding and information documented on when something is a trait and an 'OperationProperty' as well - for the benefit of those adding traits in the future. This should go into the doc comment on 'OperationProperty'. I'm assuming you aren't blocked by this revision.

We are in a situation where we have upstream code for IR that has unclear/unspecified behavior, which was pointed out by reviewers. I consider this to be an issue similar to a new bug being introduced by a change, i.e. we may want to revert the offending change. The present diff specifies this behavior, so I consider it a fix to a problem upstream. As such, it should land once accepted instead being blocked on a missing comment, that has been missing for more than a year.

There is still a blocking review on this - I don't think I can commit it yet.

Most traits are not an OperationProperty, it suffices to have a quick look in OpBase.td and see ~30 different traits being used. While we should add documentation to this enum

This justification doesn't make much sense, and can't be the basis to make a choice for a new trait. The traits are vastly different in what they mean.

(ping @rriddle), this is not precluding you from just defining a trait and landing it.

Just using a trait that doesn't use an OperationProperty bit is pretty straightforward, but simply landing the patch quickly isn't the point. What's more important here is to have the understanding and information documented on when something is a trait and an 'OperationProperty' as well - so that future contributors adding traits do the right thing. This should go into the doc comment on 'OperationProperty'. I'm assuming you aren't blocked by this revision.

In reality I've considered just removing OperationProperty. No new fields have been added(some have been removed) after I added the opaque hasTrait, because there is no need to try and bake all of these things into operation properties. The only benefit to having OperationProperty is that it is O(1) to check if an operation has a specific trait, but there are traits querying more frequently that aren't in that list.

mlir/docs/Traits.md
140

Add the -- ODS class, see above.

mlir/include/mlir/IR/OpDefinition.h
1039

What happens if the operation has a variadic number of regions?

mlir/include/mlir/IR/Operation.h
435 ↗(On Diff #256324)

This seems unrelated.

rriddle accepted this revision.Apr 9 2020, 10:54 AM
This revision is now accepted and ready to land.Apr 9 2020, 10:54 AM

Most traits are not an OperationProperty, it suffices to have a quick look in OpBase.td and see ~30 different traits being used. While we should add documentation to this enum

This justification doesn't make much sense, and can't be the basis to make a choice for a new trait. The traits are vastly different in what they mean.

(ping @rriddle), this is not precluding you from just defining a trait and landing it.

Just using a trait that doesn't use an OperationProperty bit is pretty straightforward, but simply landing the patch quickly isn't the point. What's more important here is to have the understanding and information documented on when something is a trait and an 'OperationProperty' as well - so that future contributors adding traits do the right thing. This should go into the doc comment on 'OperationProperty'. I'm assuming you aren't blocked by this revision.

In reality I've considered just removing OperationProperty. No new fields have been added(some have been removed) after I added the opaque hasTrait, because there is no need to try and bake all of these things into operation properties. The only benefit to having OperationProperty is that it is O(1) to check if an operation has a specific trait, but there are traits querying more frequently that aren't in that list.

Another thing to mention, is that I have been considering splitting the huge OpDefinitions file into a better sliced traits list. Interfaces have already been split into Interfaces/ so it makes sense to move some of the different categories of traits there. In that future, only a few traits would remain within IR/ which would give a better sense of what is "core" to operations.

bondhugula marked 2 inline comments as done.Apr 9 2020, 11:02 AM
bondhugula added inline comments.
mlir/include/mlir/IR/OpDefinition.h
1039

That should be fine. The allocations of each are still to be freed when control transfers from that respective region to the op.

rriddle added inline comments.Apr 9 2020, 11:06 AM
mlir/include/mlir/IR/OpDefinition.h
1039

Can we remove this check then? Otherwise, specific forms of operations would be legal and others not.

bondhugula marked an inline comment as done.Apr 9 2020, 11:15 AM

Most traits are not an OperationProperty, it suffices to have a quick look in OpBase.td and see ~30 different traits being used. While we should add documentation to this enum

This justification doesn't make much sense, and can't be the basis to make a choice for a new trait. The traits are vastly different in what they mean.

(ping @rriddle), this is not precluding you from just defining a trait and landing it.

Just using a trait that doesn't use an OperationProperty bit is pretty straightforward, but simply landing the patch quickly isn't the point. What's more important here is to have the understanding and information documented on when something is a trait and an 'OperationProperty' as well - so that future contributors adding traits do the right thing. This should go into the doc comment on 'OperationProperty'. I'm assuming you aren't blocked by this revision.

In reality I've considered just removing OperationProperty. No new fields have been added(some have been removed) after I added the opaque hasTrait, because there is no need to try and bake all of these things into operation properties. The only benefit to having OperationProperty is that it is O(1) to check if an operation has a specific trait, but there are traits querying more frequently that aren't in that list.

This is exactly what I was trying to find out - I think I now recall this from a discussion/question I had on the op interfaces proposal last year: there are sort of three tiers: op interfaces, traits, and operation properties. Isn't the O(1) check still useful (for eg. have a O large constant wouldn't be good at the innermost loop of say CSE like opts where you'd check whether the operands commute)? You should just have the top three over there? Isn't the number of bits preferably restricted to three?

bondhugula added inline comments.Apr 9 2020, 11:22 AM
mlir/include/mlir/IR/OpDefinition.h
1039

Ops that always have zero regions can't meaningfully have scoped allocations. We could just allow it for VariadicRegions instead by basing this check on the ZeroRegion trait?

rriddle added inline comments.Apr 9 2020, 11:45 AM
mlir/include/mlir/IR/OpDefinition.h
1039

I was wondering about the situations where we may end up in such a case, e.g. a switch-like dispatch operation that ends up having no cases(either by optimization, or generated by the front end). I would just rather avoid the situation that an optimization pass ends up generating a verifier failure for otherwise valid IR. Debugging that seems like it would be a pain.

ZeroRegion trait SGTM.

bondhugula updated this revision to Diff 256361.Apr 9 2020, 1:01 PM
bondhugula marked 5 inline comments as done.

Address review comments; use ZeroRegion trait + other minor fixes.

This revision was automatically updated to reflect the committed changes.