[Mlir-commits] [mlir] [MLIR] Adopt HasAncestor trait in omp.workshare.loop_wrapper and emitc.get_field (PR #197911)

[llvmlistbot at llvm.org]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir-openmp

Author: Berke Ates (Berke-Ates)

<details>
<summary>Changes</summary>

Following #<!-- -->195447 which added the `HasAncestor` trait, replace the ancestor-existence checks in two ops:
- `omp.workshare.loop_wrapper`
- `emitc.get_field`
  

---
Full diff: https://github.com/llvm/llvm-project/pull/197911.diff


6 Files Affected:

- (modified) mlir/include/mlir/Dialect/EmitC/IR/EmitC.td (+1-2) 
- (modified) mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td (+2-3) 
- (modified) mlir/lib/Dialect/EmitC/IR/EmitC.cpp (-8) 
- (modified) mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp (-6) 
- (modified) mlir/test/Dialect/EmitC/invalid_ops.mlir (+2-2) 
- (modified) mlir/test/Dialect/OpenMP/invalid.mlir (+1-1) 


``````````diff
diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
index deb138225c643..b443faba36486 100644
--- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
+++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
@@ -1819,7 +1819,7 @@ def EmitC_FieldOp : EmitC_Op<"field", [Symbol]> {
 }
 
 def EmitC_GetFieldOp
-    : EmitC_Op<"get_field", [Pure,
+    : EmitC_Op<"get_field", [Pure, HasAncestor<"ClassOp">,
                              DeclareOpInterfaceMethods<SymbolUserOpInterface>,
                              CExpressionInterface]> {
   let summary = "Obtain access to a field within a class instance";
@@ -1837,7 +1837,6 @@ def EmitC_GetFieldOp
   let arguments = (ins FlatSymbolRefAttr:$field_name);
   let results = (outs EmitCType:$result);
   let assemblyFormat = "$field_name `:` type($result) attr-dict";
-  let hasVerifier = 1;
 
   let extraClassDeclaration = [{
     bool hasSideEffects() {
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
index ff880755b63a6..da60a304429e8 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -635,8 +635,8 @@ def WorkshareOp : OpenMP_Op<"workshare", traits = [
 }
 
 def WorkshareLoopWrapperOp : OpenMP_Op<"workshare.loop_wrapper", traits = [
-    DeclareOpInterfaceMethods<LoopWrapperInterface>, NoTerminator,
-    RecursiveMemoryEffects, SingleBlock
+    DeclareOpInterfaceMethods<LoopWrapperInterface>, HasAncestor<"WorkshareOp">,
+    NoTerminator, RecursiveMemoryEffects, SingleBlock
   ], singleRegion = true> {
   let summary = "contains loop nests to be parallelized by workshare";
   let description = [{
@@ -648,7 +648,6 @@ def WorkshareLoopWrapperOp : OpenMP_Op<"workshare.loop_wrapper", traits = [
     OpBuilder<(ins), [{ build($_builder, $_state, {}); }]>
   ];
   let assemblyFormat = "$region attr-dict";
-  let hasVerifier = 1;
   let hasRegionVerifier = 1;
 }
 
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index 36394e67008da..fd99c090f73f7 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -1706,14 +1706,6 @@ LogicalResult FieldOp::verify() {
 // GetFieldOp
 //===----------------------------------------------------------------------===//
 
-LogicalResult GetFieldOp::verify() {
-  auto parentClassOp = getOperation()->getParentOfType<emitc::ClassOp>();
-  if (!parentClassOp.getOperation())
-    return emitOpError(" must be nested within an emitc.class operation");
-
-  return success();
-}
-
 LogicalResult GetFieldOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
   mlir::FlatSymbolRefAttr fieldNameAttr = getFieldNameAttr();
   FieldOp fieldOp =
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index ecf71480201f7..3b6c47fda4ce5 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -3143,12 +3143,6 @@ void WorkshareOp::build(OpBuilder &builder, OperationState &state,
 // WorkshareLoopWrapperOp
 //===----------------------------------------------------------------------===//
 
-LogicalResult WorkshareLoopWrapperOp::verify() {
-  if (!(*this)->getParentOfType<WorkshareOp>())
-    return emitOpError() << "must be nested in an omp.workshare";
-  return success();
-}
-
 LogicalResult WorkshareLoopWrapperOp::verifyRegions() {
   if (isa_and_nonnull<LoopWrapperInterface>((*this)->getParentOp()) ||
       getNestedWrapper())
diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir
index 0d878e90cdf0c..331f282294bae 100644
--- a/mlir/test/Dialect/EmitC/invalid_ops.mlir
+++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir
@@ -718,14 +718,14 @@ emitc.field @testField : !emitc.array<1xf32>
 
 // -----
 
-// expected-error @+1 {{'emitc.get_field' op  must be nested within an emitc.class operation}}
+// expected-error @+1 {{'emitc.get_field' op expects ancestor op 'emitc.class'}}
 %1 = emitc.get_field @testField : !emitc.array<1xf32>
 
 // -----
 
 emitc.func @testMethod() {
   %0 = "emitc.constant"() <{value = 0 : index}> : () -> !emitc.size_t
-  // expected-error @+1 {{'emitc.get_field' op  must be nested within an emitc.class operation}}
+  // expected-error @+1 {{'emitc.get_field' op expects ancestor op 'emitc.class'}}
   %1 = get_field @testField : !emitc.array<1xf32>
   %2 = subscript %1[%0] : (!emitc.array<1xf32>, !emitc.size_t) -> !emitc.lvalue<f32>
   return
diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir
index f22b05c6c9a46..304d3222853e4 100644
--- a/mlir/test/Dialect/OpenMP/invalid.mlir
+++ b/mlir/test/Dialect/OpenMP/invalid.mlir
@@ -3640,7 +3640,7 @@ func.func @not_wrapper() {
 
 // -----
 func.func @missing_workshare(%idx : index) {
-  // expected-error @below {{must be nested in an omp.workshare}}
+  // expected-error @below {{'omp.workshare.loop_wrapper' op expects ancestor op 'omp.workshare'}}
   omp.workshare.loop_wrapper {
     omp.loop_nest (%iv) : index = (%idx) to (%idx) step (%idx) {
       omp.yield

``````````

</details>


https://github.com/llvm/llvm-project/pull/197911