[PATCH] D77787: [MLIR] Introduce a trait that defines a new scope for auto allocation

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

In D77787#1972365 <https://reviews.llvm.org/D77787#1972365>, @rriddle wrote:

> In D77787#1972235 <https://reviews.llvm.org/D77787#1972235>, @bondhugula wrote:
>
> > In D77787#1971994 <https://reviews.llvm.org/D77787#1971994>, @ftynse wrote:
> >
> > > 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77787/new/

https://reviews.llvm.org/D77787