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

Thu Apr 9 11:15:10 PDT 2020

bondhugula marked an inline comment as done.
bondhugula 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.

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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77787