[mlir] Fix attaching side effects on `FlatSymbolRefAttr`

[zero9178]

Summary

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.

[mehdi_amini]

Can we add one such op in the test dialect? If the op in mlir/test/mlir-tblgen/op-side-effects.td were there we'd have caught the compile-time failure...

[zero9178]

• Add Op instance in Test Dialect to test compilation
• Use the proper getter name for the attribute to work independent of the dialects accessor prefix

[zero9178]

Test remaining uses of Arg in Test dialect as well. This should test compilation of the same features as in test/mlir-tblgen/op-side-effects.td now

[jpienaar]

Thanks!

mlir/test/lib/Dialect/Test/TestOps.td

 def TestDefaultStrAttrHasValueOp : TEST_Op<"has_str_value"> {
   let arguments = (ins DefaultValuedStrAttr<StrAttr, "">:$value);
   let assemblyFormat = "attr-dict";
 }
 
 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

mlir/test/mlir-tblgen/op-side-effects.td

 }
 
 def SideEffectOpB : TEST_Op<"side_effect_op_b",
     [MemoryEffects<[MemWrite<CustomResource>]>]>;
 
 // 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))
 // CHECK:     effects.emplace_back(::mlir::MemoryEffects::Allocate::get(), value, CustomResource::get());
 
 // CHECK: void SideEffectOpB::getEffects
 // CHECK:   effects.emplace_back(::mlir::MemoryEffects::Write::get(), CustomResource::get());

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

for (auto &location : it.second) {
       StringRef effect = location.effect.getName();
       StringRef resource = location.effect.getResource();
       if (location.kind == EffectKind::Static) {
         // A static instance has no attached value.
         body << llvm::formatv(addEffectCode, effect, "", resource).str();
       } 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();
         }
       } else {
         // Otherwise this is an operand/result, so we need to attach the Value.
         body << "  for (::mlir::Value value : getODS"
              << (location.kind == EffectKind::Operand ? "Operands" : "Results")
              << "(" << location.index << "))\n  "