The module op’s verifier fails when an attribute with a name is missing a ‘.’ (code below). I’m trying to understand what the rationale for this is? First, the verifier is anyway not checking whether the prefix is a registered dialect. Secondly, the builtin dialect itself has an empty name, and so an unprefixed name would be a built-in dialect specific attribute. So, both the comment as well as the error is out of line with this behavior. AFAIR, all operations normally allow arbitrary attribute names.
error: 'module' op can only contain dialect-specific attributes, found: 'foo'
static LogicalResult verify(ModuleOp op) {
// Check that none of the attributes are non-dialect attributes, except for
// the symbol related attributes.
for (auto attr : op.getAttrs()) {
if (!attr.first.strref().contains('.') &&
!llvm::is_contained(
ArrayRef<StringRef>{mlir::SymbolTable::getSymbolAttrName(),
mlir::SymbolTable::getVisibilityAttrName()},
attr.first.strref()))
return op.emitOpError()
<< "can only contain dialect-specific attributes, found: '"
<< attr.first << "'";
}
return success();
}
I’m not exactly sure, but I think this is an instance of a general principle here: there are two kinds of attributes in MLIR, operation defined attributes and “other”.
Operation defined attributes are things like the “element index” in a tuple extraction - something inherent to the operation that all things touching the operation should be expected to understand.
Dialect attributes are the “other” escape hatch. They aren’t always super principled :-), but the idea is that we need/want to be able to splatter attributes on ops that don’t expect them for domain specific reasons. This is fraught with peril for the same reason that metadata is in LLVM IR, but is useful nonetheless.
This verifier could be made more strict by also checking if dialect is registered (except where the user has allowed for unregistered dialects), and module is more strict than general op here and given how widely it is used I think that is good.
Built-in dialect doesn’t and won’t have dialect attributes on Module. In the case that it is ever needed it would be an explicit op attribute on the module op instead (e.g., we’d change the op definition).
The problem is that if we let external attributes be added with no context, it quickly becomes unwieldy and can lead to undesirable behavior. For example, external entity attributes may clash with attributes managed by the operation itself which could lead to miscompiles/crashes/etc. This is why we ended up with the two separate classes of attributes to clearly demarcate where the context of the attribute originates from, labeled dialect and dependent(which are discussed in the LangRef). (Though dialect is kind of weird now that dialects can define their own attributes. The classes apply to named attributes and not attribute values.)
For the second bit about the empty namespace’d builtin dialect, I’m actually of the opinion that we should change that. The current builtin dialect and it’s weird “hidden but not” design has resulted in some undesirable hacks and other annoyances, which is why I started refactoring it to be more user facing. The desire is that by the end, we can clean up some of these loose ends(e.g. empty namespace) and make the restrictions on dialects tighter.
But it’s weird that it’s disallowed for ModuleOp but allowed for say FuncOps? One may need these user-defined attributes (I prefer using this name) on ModuleOp for the same reason they are useful for FuncOp. And there isn’t in many cases a dialect associated with these attributes. (I’m still not sure whether the attribute name having a dialect prefix here, as referred to by the error message, is the same as “dialect attribute values” in LangRef). For eg. functions coming in from TF models use function attributes to encode input/output tensor info. Although such attributes create difficulties that you and Chris mention above, I can’t see how it’s better than not having them. They are the only simple / best way to model what they do in many cases.
On this note, I just looked at MLIR Language Reference - MLIR – is this what the error message is referring to? But it’s not checking whether those have been defined.
The section I was referring to was the one directly linked, i.e.:
There are two main classes of attributes: dependent and dialect. Dependent attributes derive their structure and meaning from what they are attached to; e.g., the meaning of the index attribute on a dim operation is defined by the dim operation. Dialect attributes, on the other hand, derive their context and meaning from a specific dialect. An example of a dialect attribute may be a swift.self function argument attribute that indicates an argument is the self/context parameter. The context of this attribute is defined by the swift dialect and not the function argument.
This doesn’t prevent external entities putting attributes on operations like ModuleOp or FuncOp. It merely prevents using a dependent attribute name. More specifically, users should be doing tf.my_attr instead of my_attr. That is the difference between dialect and dependent, dialect attribute names have the dialect namespace prefixing the attribute name.
Isn’t the error message then just asking for dialect attribute names as opposed to dialect attribute values? Dialect attribute is used interchangeably with Dialect attribute values in the description in LangRef, and my confusion is due to that.
Yes. The naming is definitely problematic. We should fix the LangRef/error/etc. to make a strong distinction between attribute names and attribute values, or find some other name for “dialect attribute names”.
I’m not sure external clearly conveys “external to op” or that the name has to be dialect prefixed. It also doesn’t seem to help with the error message here. I was going to suggest:
Changing the error message to use “dialect-prefixed attribute names”. I think the term “dialect attributes” should just be banned in this context?! It’s not an attribute of the dialect (it’s an attribute of the op) nor is the op (that it’s attached to) necessarily from the dialect! It’s really confusing.