[MLIR] Disallow `sym_visibility`, `sym_name` and `type` attributes in the parsed attribute dictionary.

[jurahul]

Summary

As above.

[rriddle]

mlir/include/mlir/IR/OpImplementation.h

528

Why do you need any of this? Can't you just check for these after parsing?

[jurahul]

This is in preparation of the visibility related changes discussed in https://llvm.discourse.group/t/external-function-declaration-has-changed-help-needed/2357. This change disallows attributes that sym_visibility/sym_name/type etc which are parsed/inferred from other places to be present in the attribute dictionary. This way, for the follow on changes, the function parser has just one parsed visibility value (parsed as keyword private/public/nested) to consider instead of trying to also look into the parsed attribute dictionary.

[rriddle]

In D94200#2483226, @jurahul wrote:

This is in preparation of the visibility related changes discussed in https://llvm.discourse.group/t/external-function-declaration-has-changed-help-needed/2357. This change disallows attributes that sym_visibility/sym_name/type etc which are parsed/inferred from other places to be present in the attribute dictionary. This way, for the follow on changes, the function parser has just one parsed visibility value (parsed as keyword private/public/nested) to consider instead of trying to also look into the parsed attribute dictionary.

I don't see why you can't just check these in FunctionImplementation.cpp. Why do you need to add more flags to the Parser methods?

[rriddle]

In D94200#2483234, @rriddle wrote:

In D94200#2483226, @jurahul wrote:

This is in preparation of the visibility related changes discussed in https://llvm.discourse.group/t/external-function-declaration-has-changed-help-needed/2357. This change disallows attributes that sym_visibility/sym_name/type etc which are parsed/inferred from other places to be present in the attribute dictionary. This way, for the follow on changes, the function parser has just one parsed visibility value (parsed as keyword private/public/nested) to consider instead of trying to also look into the parsed attribute dictionary.

I don't see why you can't just check these in FunctionImplementation.cpp. Why do you need to add more flags to the Parser methods?

For this situation, having FunctionImplementation emit the error seems better given that "attribute is not allowed in attribute dictionary" does not provide much context on what the error is, or how it should be fixed. Can we provide a better error message instead?

[jurahul]

Simplify by moving check in parseFunctionLikeOp

[rriddle]

mlir/lib/IR/FunctionImplementation.cpp

212

Can you add a bit more context here?

Though maybe this is just me getting confused thinking about it. When I think of the attribute dictionary, I think of the complete dictionary on the operation and not just this specific one. We could provide specialized error messages for these attributes:

sym_visibility attribute should not be specified in the attribute dictionary, and instead be specified before the name of the function or something.

mlir/lib/Parser/AttributeParser.cpp

270

Spurious change?

[jurahul]

mlir/lib/IR/FunctionImplementation.cpp

211

Still thinking of a clear error message: inferred attribute XXX is not allowed.. or implicit attribute XXX is not allowed?

[jurahul]

mlir/lib/IR/FunctionImplementation.cpp

212

Yes, when I say "attribute dictionary" I mean the parsed one, and not the complete one with parsed + inferred attributes. To be clear, we would need to specialize the error message per attribute. Let me try that.

[rriddle]

LGTM after adding error messages. Thanks!

mlir/lib/IR/FunctionImplementation.cpp

211

Something like sounds good. XXX is an inferred attribute and should not appear in the explicit attribute dictionary.

[jurahul]

Better error messages

[jurahul]

Make error message concise.

mlir/include/mlir/IR/OpImplementation.h

ParseResult parseAttribute(AttrType &result, Type type, StringRef attrName,
     result = attr.dyn_cast<AttrType>();
     if (!result)
       return emitError(loc, "invalid kind of attribute specified");
 
     attrs.append(attrName, result);
     return success();
   }
 
-  /// Parse a named dictionary into 'result' if it is present.
-  virtual ParseResult parseOptionalAttrDict(NamedAttrList &result) = 0;
+  /// Parse a named dictionary into 'result' if it is present. Fails parsing if
+  /// any of the attributes in the dictionary is in `disallowed`.
+  virtual ParseResult
+  parseOptionalAttrDict(NamedAttrList &result,
+                        ArrayRef<StringRef> disallowed = {}) = 0;
 
   /// Parse a named dictionary into 'result' if the `attributes` keyword is
-  /// present.
+  /// present. Fails parsing if any of the attributes in the dictionary is in
+  /// `disallowed`.
   virtual ParseResult
-  parseOptionalAttrDictWithKeyword(NamedAttrList &result) = 0;
+  parseOptionalAttrDictWithKeyword(NamedAttrList &result,
+                                   ArrayRef<StringRef> disallowed = {}) = 0;
 
   /// Parse an affine map instance into 'map'.
   virtual ParseResult parseAffineMap(AffineMap &map) = 0;
 
   /// Parse an integer set instance into 'set'.
   virtual ParseResult printIntegerSet(IntegerSet &set) = 0;
 
   //===--------------------------------------------------------------------===//

mlir/lib/IR/FunctionImplementation.cpp

if (auto type = funcTypeBuilder(builder, argTypes, resultTypes,
                                   impl::VariadicFlag(isVariadic), errorMessage))
     result.addAttribute(getTypeAttrName(), TypeAttr::get(type));
   else
     return parser.emitError(signatureLocation)
            << "failed to construct function type"
            << (errorMessage.empty() ? "" : ": ") << errorMessage;
 
   // If function attributes are present, parse them.
-  if (parser.parseOptionalAttrDictWithKeyword(result.attributes))
+  if (parser.parseOptionalAttrDictWithKeyword(
+          result.attributes,
+          {SymbolTable::getSymbolAttrName(),
+           SymbolTable::getVisibilityAttrName(), getTypeAttrName()}))
     return failure();
 
   // Add the attributes to the function arguments.
   assert(argAttrs.size() == argTypes.size());
   assert(resultAttrs.size() == resultTypes.size());
   addArgAndResultAttrs(builder, result, argAttrs, resultAttrs);
 
   // Parse the optional function body. The printer will not print the body if
   // its empty, so disallow parsing of empty body in the parser.
   auto *body = result.addRegion();
   llvm::SMLoc loc = parser.getCurrentLocation();
   OptionalParseResult parseResult = parser.parseOptionalRegion(
       *body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes,
       /*enableNameShadowing=*/false);
   if (parseResult.hasValue()) {
     if (failed(*parseResult))
       return failure();
     // Function body was parsed, make sure its not empty.

mlir/lib/Parser/AttributeParser.cpp

 }
 
 /// Attribute dictionary.
 ///
 ///   attribute-dict ::= `{` `}`
 ///                    | `{` attribute-entry (`,` attribute-entry)* `}`
 ///   attribute-entry ::= (bare-id | string-literal) `=` attribute-value
 ///
-ParseResult Parser::parseAttributeDict(NamedAttrList &attributes) {
+ParseResult Parser::parseAttributeDict(NamedAttrList &attributes,
+                                       ArrayRef<StringRef> disallowed) {
   if (parseToken(Token::l_brace, "expected '{' in attribute dictionary"))
     return failure();
 
   llvm::SmallDenseSet<Identifier> seenKeys;
+  const llvm::SmallDenseSet<StringRef> disallowedKeys(disallowed.begin(),
+                                                      disallowed.end());
   auto parseElt = [&]() -> ParseResult {
     // The name of an attribute can either be a bare identifier, or a string.
     Optional<Identifier> nameId;
     if (getToken().is(Token::string))
       nameId = builder.getIdentifier(getToken().getStringValue());
     else if (getToken().isAny(Token::bare_identifier, Token::inttype) ||
              getToken().isKeyword())
       nameId = builder.getIdentifier(getTokenSpelling());
     else
       return emitError("expected attribute name");
+
+    if (disallowedKeys.contains(nameId->strref()))
+      return emitError("attribute '")
+             << *nameId << "' is not allowed in attribute dictionary";
     if (!seenKeys.insert(*nameId).second)
       return emitError("duplicate key '")
-             << *nameId << "' in dictionary attribute";
+             << *nameId << "' in attribute dictionary";
     consumeToken();
 
     // Lazy load a dialect in the context if there is a possible namespace.
     auto splitName = nameId->strref().split('.');
     if (!splitName.second.empty())
       getContext()->getOrLoadDialect(splitName.first);
 
     // Try to parse the '=' for the attribute value.

mlir/lib/Parser/Parser.h

OptionalParseResult parseOptionalAttributeWithToken(Token::Kind kind,
 
     if (Attribute parsedAttr = parseAttribute(type)) {
       attr = parsedAttr.cast<AttributeT>();
       return success();
     }
     return failure();
   }
 
-  /// Parse an attribute dictionary.
-  ParseResult parseAttributeDict(NamedAttrList &attributes);
+  /// Parse an attribute dictionary. Fails parsing if any of the attributes in
+  /// the dictionary is in `disallowed`.
+  ParseResult parseAttributeDict(NamedAttrList &attributes,
+                                 ArrayRef<StringRef> disallowed = {});
 
   /// Parse an extended attribute.
   Attribute parseExtendedAttr(Type type);
 
   /// Parse a float attribute.
   Attribute parseFloatAttr(Type type, bool isNegative);
 
   /// Parse a decimal or a hexadecimal literal, which can be either an integer

mlir/lib/Parser/Parser.cpp

OptionalParseResult parseOptionalAttribute(ArrayAttr &result, Type type,
     return parseOptionalAttributeAndAddToList(result, type, attrName, attrs);
   }
   OptionalParseResult parseOptionalAttribute(StringAttr &result, Type type,
                                              StringRef attrName,
                                              NamedAttrList &attrs) override {
     return parseOptionalAttributeAndAddToList(result, type, attrName, attrs);
   }
 
-  /// Parse a named dictionary into 'result' if it is present.
-  ParseResult parseOptionalAttrDict(NamedAttrList &result) override {
+  /// Parse a named dictionary into 'result' if it is present. Fails parsing if
+  /// any of the attributes in the dictionary is in `disallowed`.
+  ParseResult parseOptionalAttrDict(NamedAttrList &result,
+                                    ArrayRef<StringRef> disallowed) override {
     if (parser.getToken().isNot(Token::l_brace))
       return success();
-    return parser.parseAttributeDict(result);
+    return parser.parseAttributeDict(result, disallowed);
   }
 
   /// Parse a named dictionary into 'result' if the `attributes` keyword is
-  /// present.
-  ParseResult parseOptionalAttrDictWithKeyword(NamedAttrList &result) override {
+  /// present. Fails parsing if any of the attributes in the dictionary is in
+  /// `disallowed`.
+  ParseResult
+  parseOptionalAttrDictWithKeyword(NamedAttrList &result,
+                                   ArrayRef<StringRef> disallowed) override {
     if (failed(parseOptionalKeyword("attributes")))
       return success();
-    return parser.parseAttributeDict(result);
+    return parser.parseAttributeDict(result, disallowed);
   }
 
   /// Parse an affine map instance into 'map'.
   ParseResult parseAffineMap(AffineMap &map) override {
     return parser.parseAffineMapReference(map);
   }
 
   /// Parse an integer set instance into 'set'.

mlir/test/Dialect/Tosa/inlining.mlir

^bb0(%arg3: tensor<f32>, %arg4: tensor<f32>):  // no predecessors
     "tosa.yield"(%1) : (tensor<f32>) -> ()
   },  {
   ^bb0(%arg3: tensor<f32>, %arg4: tensor<f32>):  // no predecessors
     %1 = call @sub(%arg3, %arg4) : (tensor<f32>, tensor<f32>) -> tensor<f32>
     "tosa.yield"(%1) : (tensor<f32>) -> ()
   }) : (tensor<i1>, tensor<f32>, tensor<f32>) -> tensor<f32>
   return %0 : tensor<f32>
 }
-func @add(%arg0: tensor<f32>, %arg1: tensor<f32>) -> tensor<f32> attributes {sym_visibility = "private"} {
+func private @add(%arg0: tensor<f32>, %arg1: tensor<f32>) -> tensor<f32> {
   %0 = "tosa.add"(%arg0, %arg1) : (tensor<f32>, tensor<f32>) -> tensor<f32>
   return %0 : tensor<f32>
 }
-func @sub(%arg0: tensor<f32>, %arg1: tensor<f32>) -> tensor<f32> attributes {sym_visibility = "private"} {
+func private @sub(%arg0: tensor<f32>, %arg1: tensor<f32>) -> tensor<f32> {
   %0 = "tosa.sub"(%arg0, %arg1) : (tensor<f32>, tensor<f32>) -> tensor<f32>
   return %0 : tensor<f32>
 }
 
 // -----
 
 // CHECK-LABEL: func @inlined_while_fn
 func @inlined_while_fn(%arg0: tensor<i32>, %arg1: tensor<i32>, %arg2: tensor<i32>, %arg3: tensor<10xi32>) -> tensor<10xi32> {
   // Check that calls are inlined and functions eliminated:
   // CHECK-NOT: @while
   %1:4 = "tosa.while_loop"(%arg0, %arg1, %arg2, %arg3) ( {
   ^bb0(%arg4: tensor<i32>, %arg5: tensor<i32>, %arg6: tensor<i32>, %arg7: tensor<10xi32>):  // no predecessors
     %2 = call @while_cond_40(%arg4, %arg5, %arg6, %arg7) : (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>) -> tensor<i1>
     "tosa.yield"(%2) : (tensor<i1>) -> ()
   },  {
   ^bb0(%arg4: tensor<i32>, %arg5: tensor<i32>, %arg6: tensor<i32>, %arg7: tensor<10xi32>):  // no predecessors
     %2:4 = call @while_body_50(%arg4, %arg5, %arg6, %arg7) : (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>) -> (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>)
     "tosa.yield"(%2#0, %2#1, %2#2, %2#3) : (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>) -> ()
   }) : (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>) -> (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>)
   return %1#3 : tensor<10xi32>
 }
-func @while_body_50(%arg0: tensor<i32>, %arg1: tensor<i32>, %arg2: tensor<i32>, %arg3: tensor<10xi32>) -> (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>) attributes {sym_visibility = "private"} {
+func private @while_body_50(%arg0: tensor<i32>, %arg1: tensor<i32>, %arg2: tensor<i32>, %arg3: tensor<10xi32>) -> (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>) {
   %1 = "tosa.add"(%arg0, %arg1) : (tensor<i32>, tensor<i32>) -> tensor<i32>
   %2 = "tosa.add"(%arg3, %1) : (tensor<10xi32>, tensor<i32>) -> tensor<10xi32>
   return %1, %arg1, %arg2, %2: tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>
 }
-func @while_cond_40(%arg0: tensor<i32>, %arg1: tensor<i32>, %arg2: tensor<i32>, %arg3: tensor<10xi32>) -> tensor<i1> attributes {sym_visibility = "private"} {
+func private @while_cond_40(%arg0: tensor<i32>, %arg1: tensor<i32>, %arg2: tensor<i32>, %arg3: tensor<10xi32>) -> tensor<i1> {
   %0 = "tosa.greater_equal"(%arg0, %arg1) : (tensor<i32>, tensor<i32>) -> tensor<i1>
   %1 = "tosa.logical_not"(%0) : (tensor<i1>) -> tensor<i1>
   return %1 : tensor<i1>
 }

mlir/test/IR/core-ops.mlir

func @subtensor_insert(%t: tensor<8x16x4xf32>, %t2: tensor<16x32x8xf32>, %idx : index) {
 
   // CHECK: subtensor_insert
   // CHECK-SAME: tensor<8x16x4xf32> into tensor<16x32x8xf32>
   %2 = subtensor_insert %t into %t2[%c0, %idx, %c0][%idx, 4, %idx][%c1, 1, %c1]
     : tensor<8x16x4xf32> into tensor<16x32x8xf32>
 
   return
 }
-
-// CHECK-LABEL: func private @legacy_visibility_syntax
-func @legacy_visibility_syntax() attributes { sym_visibility = "private" }

mlir/test/IR/invalid-func-op.mlir

 func @f(%arg0: i64) -> (i64 {test.invalid_attr}) {
   return %arg0 : i64
 }
 
 // -----
 
 // expected-error@+1 {{symbol declaration cannot have public visibility}}
 func @invalid_public_declaration()
+
+// -----
+
+// expected-error@+1 {{attribute 'sym_visibility' is not allowed in attribute dictionary}}
+func @legacy_visibility_syntax() attributes { sym_visibility = "private" }
+

mlir/test/IR/invalid.mlir

"test.out_of_range_attribute"() {
     attr = 39614081257132168796771975168 : si96
   } : () -> ()
   return
 }
 
 // -----
 
 func @duplicate_dictionary_attr_key() {
-  // expected-error @+1 {{duplicate key 'a' in dictionary attribute}}
+  // expected-error @+1 {{duplicate key 'a' in attribute dictionary}}
   "foo.op"() {a, a} : () -> ()
 }
 
 // -----
 
 // expected-error @+1 {{attribute 'attr' occurs more than once in the attribute list}}
 test.format_symbol_name_attr_op @name { attr = "xx" }