[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 ↗

(On Diff #315009)

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

211

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

263 ↗

(On Diff #315011)

Spurious change?

[jurahul]

mlir/lib/IR/FunctionImplementation.cpp

210

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

211

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

210

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/lib/IR/FunctionImplementation.cpp

mlir::impl::parseFunctionLikeOp(OpAsmParser &parser, OperationState &result,
 
   // Parse the name as a symbol.
   StringAttr nameAttr;
   if (parser.parseSymbolName(nameAttr, SymbolTable::getSymbolAttrName(),
                              result.attributes))
     return failure();
 
   // Parse the function signature.
-  auto signatureLocation = parser.getCurrentLocation();
+  llvm::SMLoc signatureLocation = parser.getCurrentLocation();
   bool isVariadic = false;
   if (parseFunctionSignature(parser, allowVariadic, entryArgs, argTypes,
                              argAttrs, isVariadic, resultTypes, resultAttrs))
     return failure();
 
   std::string errorMessage;
   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))
-    return failure();
+  NamedAttrList parsedAttributes;
+  llvm::SMLoc attributeDictLocation = parser.getCurrentLocation();
+  if (parser.parseOptionalAttrDictWithKeyword(parsedAttributes))
+    return failure();
+
+  // Disallow attributes that are inferred from elsewhere in the attribute
+  // dictionary.
+  for (StringRef disallowed :
+       {SymbolTable::getVisibilityAttrName(), SymbolTable::getSymbolAttrName(),
+        getTypeAttrName()}) {
+    if (parsedAttributes.get(disallowed))
+      return parser.emitError(attributeDictLocation, "'")
+             << disallowed
+             << "' is an inferred attribute and should not be specified in the "
+                "explicit attribute dictionary";
+  }
+  result.attributes.append(parsedAttributes);
 
   // 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.

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 {{'sym_visibility' is an inferred attribute and should not be specified in the explicit attribute dictionary}}
+func @legacy_visibility_syntax() attributes { sym_visibility = "private" }
+
+// -----
+
+// expected-error@+1 {{'sym_name' is an inferred attribute and should not be specified in the explicit attribute dictionary}}
+func private @invalid_symbol_name_attr() attributes { sym_name = "x" }
+
+// -----
+
+// expected-error@+1 {{'type' is an inferred attribute and should not be specified in the explicit attribute dictionary}}
+func private @invalid_symbol_type_attr() attributes { type = "x" }
+