[PATCH] D80358: [MLIR] Add OpTrait::DominanceFreeScope

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula requested changes to this revision.
bondhugula added a comment.
This revision now requires changes to proceed.

A few comments - most of them are minor. Shouldn't the pass classes be changed to allow passes to skip dominance-free region ops? This could be marked a TODO. I don't see any changes to properlyDominates/dominates, etc. methods. How does their behavior change?
Commit summary typo: are not requires -> are not required



================
Comment at: mlir/include/mlir/IR/OpBase.td:1639
 
+// Op contains regions where dominance is not a required property
+def DominanceFreeScope : NativeOpTrait<"DominanceFreeScope">;
----------------
Nit: terminate with a period please.


================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1039-1043
+/// A trait of region-holding operations that do not require the
+/// SSA-Dominance property.  This is useful for scopes with semantics
+/// that doesn't correspond with the traditional notion of a CDFG.
+/// For more details, see `Traits.md#DominanceFreeScope`.
+template <typename ConcreteType>
----------------
Reflow.


================
Comment at: mlir/lib/IR/Verifier.cpp:64-65
+  LogicalResult verifyDominance(Region &region, bool inDominanceScope);
   LogicalResult verifyDominance(Operation &op);
+  LogicalResult verifyDominanceOfContainedRegions(Operation &op);
 
----------------
This needs a comment - otherwise it may not be clear to new readers whether the former check is a strict superset of the latter.


================
Comment at: mlir/lib/IR/Verifier.cpp:225
 
-LogicalResult OperationVerifier::verifyDominance(Region &region) {
+LogicalResult OperationVerifier::verifyDominance(Region &region, bool inDominanceScope) {
   // Verify the dominance of each of the held operations.
----------------
inDominanceScope sounds odd. Switch to isDominanceFreeScope and negate?


================
Comment at: mlir/lib/IR/Verifier.cpp:267
+LogicalResult OperationVerifier::verifyDominanceOfContainedRegions(Operation &op) {
+  bool inDominanceScope = !op.hasTrait<OpTrait::DominanceFreeScope>();
   for (auto &region : op.getRegions())
----------------
Perhaps drop the !, and nDominanceScope -> isDominanceFreeScope?


================
Comment at: mlir/test/IR/traits.mlir:356
+
+func @succededDominanceFreeScope() -> () {
+  test.dominance_free_scope {
----------------
Typo -> succeeded
Nit: test case names normally just use snake case.


================
Comment at: mlir/test/IR/traits.mlir:360-367
+    br ^bb4     // CHECK:   br ^bb2
+^bb2:         // CHECK: ^bb1:   // pred: ^bb1
+    br ^bb2     // CHECK:   br ^bb1
+^bb4:         // CHECK: ^bb2:   // pred: ^bb0
+    %1 = "foo"() : ()->i64 // CHECK: [[VAL3]] = "foo"() : () -> i64
+  }
+  return
----------------
Can you align the CHECKs please? It's really not very readable.


================
Comment at: mlir/test/IR/traits.mlir:369
+
+// -----
+
----------------
Could you also add a single block straightline IR test case that doesn't follow dominance?


================
Comment at: mlir/test/IR/traits.mlir:371
+
+func @illegalCDFGInsideDominanceFreeScope() -> () {
+  test.dominance_free_scope {
----------------
Could you add a comment on what this is testing?


================
Comment at: mlir/test/IR/traits.mlir:382
+      %1 = "foo"() : ()->i64   // expected-note {{operand defined here}}
+		return %2#1 : i1
+    }
----------------
Misaligned.


================
Comment at: mlir/test/IR/traits.mlir:387
+  return
+}            // CHECK: }
----------------
I think a negative test case in which there is no def for a value (only uses) inside a dominance free op is missing.


================
Comment at: mlir/test/lib/Dialect/Test/TestDialect.cpp:208
+static ParseResult parseDominanceFreeScopeOp(OpAsmParser &parser,
+                                      OperationState &result) {
+  // Parse the body region, and reuse the operand info as the argument info.
----------------
Format - misaligned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80358