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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.

Looks fine to me with comments adressed



================
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
----------------
Nit: can we make this an actual markdown list?


================
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");
----------------
Shouldn't we rather change that `op` actually has regions? An op may not have `ZeroRegion` trait and still have no associated 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);
----------------
Did you mean `parentOp->hasTrait<PolyhedralScope>()`  instead of isolated from above?


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78863