[Mlir-commits] [mlir] 52b8fe9 - [mlir] Fix attaching side effects on `FlatSymbolRefAttr`

[Markus Böck]

Author: Markus Böck
Date: 2022-01-13T19:57:01+01:00
New Revision: 52b8fe9b6e014184986a6d1b69ded60926d94c23

URL: https://github.com/llvm/llvm-project/commit/52b8fe9b6e014184986a6d1b69ded60926d94c23
DIFF: https://github.com/llvm/llvm-project/commit/52b8fe9b6e014184986a6d1b69ded60926d94c23.diff

LOG: [mlir] Fix attaching side effects on `FlatSymbolRefAttr`

The names of the generated attribute getters for ops changed some time ago. The method created from the attribute name returns the return type and an additional method of the same name with Attr as suffix is generated which returns the actual attribute as its storage type.

The code generating effects however was using the methods without the Attr suffix, which is a problem in the case of FlatSymbolRefAttr as it has a return type of llvm::StringRef. This would lead to compilation errors as the constructor of SideEffects::EffectInstance expects a SymbolRefAttr in this case.

This patch simply fixes the generated effects code to use the Attr suffixed getter to get the actual storage type of the attribute.

Differential Revision: https://reviews.llvm.org/D117194

Added: 
    

Modified: 
    mlir/test/lib/Dialect/Test/TestOps.td
    mlir/test/mlir-tblgen/op-side-effects.td
    mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index 0a177e9eb927..cc9c950ea73f 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -2580,4 +2580,24 @@ def TestDefaultStrAttrHasValueOp : TEST_Op<"has_str_value"> {
 def : Pat<(TestDefaultStrAttrNoValueOp $value),
           (TestDefaultStrAttrHasValueOp ConstantStrAttr<StrAttr, "foo">)>;
 
+//===----------------------------------------------------------------------===//
+// Test Ops with effects
+//===----------------------------------------------------------------------===//
+
+def TestResource : Resource<"TestResource">;
+
+def TestEffectsOpA : TEST_Op<"op_with_effects_a"> {
+    let arguments = (ins
+        Arg<Variadic<AnyMemRef>, "", [MemRead]>,
+        Arg<FlatSymbolRefAttr, "", [MemRead]>:$first,
+        Arg<SymbolRefAttr, "", [MemWrite]>:$second,
+        Arg<OptionalAttr<SymbolRefAttr>, "", [MemRead]>:$optional_symbol
+        );
+
+    let results = (outs Res<AnyMemRef, "", [MemAlloc<TestResource>]>);
+}
+
+def TestEffectsOpB : TEST_Op<"op_with_effects_b",
+    [MemoryEffects<[MemWrite<TestResource>]>]>;
+
 #endif // TEST_OPS

diff  --git a/mlir/test/mlir-tblgen/op-side-effects.td b/mlir/test/mlir-tblgen/op-side-effects.td
index 292fdfb7b3a3..ea1f00ff2979 100644
--- a/mlir/test/mlir-tblgen/op-side-effects.td
+++ b/mlir/test/mlir-tblgen/op-side-effects.td
@@ -26,8 +26,8 @@ def SideEffectOpB : TEST_Op<"side_effect_op_b",
 // CHECK: void SideEffectOpA::getEffects
 // CHECK:   for (::mlir::Value value : getODSOperands(0))
 // CHECK:     effects.emplace_back(::mlir::MemoryEffects::Read::get(), value, ::mlir::SideEffects::DefaultResource::get());
-// CHECK:   effects.emplace_back(::mlir::MemoryEffects::Read::get(), symbol(), ::mlir::SideEffects::DefaultResource::get());
-// CHECK:   effects.emplace_back(::mlir::MemoryEffects::Write::get(), flat_symbol(), ::mlir::SideEffects::DefaultResource::get());
+// CHECK:   effects.emplace_back(::mlir::MemoryEffects::Read::get(), symbolAttr(), ::mlir::SideEffects::DefaultResource::get());
+// CHECK:   effects.emplace_back(::mlir::MemoryEffects::Write::get(), flat_symbolAttr(), ::mlir::SideEffects::DefaultResource::get());
 // CHECK:   if (auto symbolRef = optional_symbolAttr())
 // CHECK:     effects.emplace_back(::mlir::MemoryEffects::Read::get(), symbolRef, ::mlir::SideEffects::DefaultResource::get());
 // CHECK:   for (::mlir::Value value : getODSResults(0))

diff  --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 8511df9c54e6..c6ffc76af952 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -2053,12 +2053,13 @@ void OpEmitter::genSideEffectInterfaceMethods() {
       } else if (location.kind == EffectKind::Symbol) {
         // A symbol reference requires adding the proper attribute.
         const auto *attr = op.getArg(location.index).get<NamedAttribute *>();
+        std::string argName = op.getGetterName(attr->name);
         if (attr->attr.isOptional()) {
-          body << "  if (auto symbolRef = " << attr->name << "Attr())\n  "
+          body << "  if (auto symbolRef = " << argName << "Attr())\n  "
                << llvm::formatv(addEffectCode, effect, "symbolRef, ", resource)
                       .str();
         } else {
-          body << llvm::formatv(addEffectCode, effect, attr->name + "(), ",
+          body << llvm::formatv(addEffectCode, effect, argName + "Attr(), ",
                                 resource)
                       .str();
         }