[PATCH] D78863: [MLIR] Introduce op trait PolyhedralScope

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

Thanks for the review!



================
Comment at: mlir/docs/Dialects/Affine.md:64
+identifiers to enable powerful analysis and transformation. An SSA value's use
+can be bound to a symbolic identifier if that SSA value is either (1) a region
+argument for an op with trait `PolyhedralScope` (eg. `FuncOp`), (2) a value
----------------
ftynse wrote:
> Nit: can we make this an actual markdown list?
Sure - that would be better. 


================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1060
+  static LogicalResult verifyTrait(Operation *op) {
+    if (op->hasTrait<ZeroRegion>())
+      return op->emitOpError("is expected to have regions");
----------------
ftynse wrote:
> Shouldn't we rather change that `op` actually has regions? An op may not have `ZeroRegion` trait and still have no associated regions.
One could imagine an op that has some instances of it with zero regions and some instances with a region. One would in such cases let the op have the PolyhedralScope trait: we don't have such an example in the code base or a scenario I can think of right away but no regions is actually no harm. So, I thought we'll only disallow if it's statically a zero region op. That said, this check should have actually been a static_assert:
```
static_assert(ConcreteType::template hasTrait<ZeroRegion>(),
                  "expected operation to have one or more regions");
```


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:135
   auto *parentOp = value.cast<BlockArgument>().getOwner()->getParentOp();
-  return isa<FuncOp>(parentOp) || isa<AffineForOp>(parentOp) ||
+  return parentOp->isKnownIsolatedFromAbove() || isa<AffineForOp>(parentOp) ||
          isa<AffineParallelOp>(parentOp);
----------------
ftynse wrote:
> Did you mean `parentOp->hasTrait<PolyhedralScope>()`  instead of isolated from above?
Thanks - forgot to update this. 


================
Comment at: mlir/test/Dialect/Affine/ops.mlir:122
+func @valid_symbol_polyhedral_scope(%n : index, %A : memref<?xf32>) {
+  test.polyhedral_scope {
+    %c1 = constant 1 : index
----------------
ftynse wrote:
> It is a bit unusual to have a dialect test depend on ops from the test dialect, but looks okay given the nature of the test.
Hmm... there is one in parser.mlir also that depends on test.isolated_region; can't think of a way to test these without the test dialect. But once there is affine.execute_region, that could replace this. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78863