[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#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.



================
Comment at: mlir/docs/Traits.md:140
+
+*   `OpTrait::AutomaticAllocationScope`
+
----------------
Add the `-- ODS class`, see above.


================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1039
+  static LogicalResult verifyTrait(Operation *op) {
+    if (op->getNumRegions() == 0)
+      return op->emitOpError("is expected to have regions");
----------------
What happens if the operation has a variadic number of regions?


================
Comment at: mlir/include/mlir/IR/Operation.h:435
 
-  /// Returns if the operation is known to be completely isolated from enclosing
-  /// regions, i.e. no internal regions reference values defined above this
-  /// operation.
+  /// Returns true if the operation is known to be completely isolated from
+  /// enclosing regions, i.e. no internal regions reference values defined above
----------------
This seems unrelated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77787