Because complex isn’t a primitive type, it is an aggregate type like structure, array etc. I’m still not aware of a problem with using ArrayAttr here with a ComplexType.
-Chris
This is what I’m asking about. What negative performance implications are you worried about here? All these things are uniqued. Are you looking to save a couple bytes on complex half float values or something? Are you actually worried about the memory usage of the compiler being a few bytes more?
I am just repeating what I got as feedback here, these are not my own concerns about performance. I don’t have much insight into MLIR and cannot reason about performance and stuff. My goal with this RFC is simply to gather feedback and an agreement on a solution 
Because complex isn’t a primitive type, it is an aggregate type like structure, array etc. I’m still not aware of a problem with using ArrayAttr here with a ComplexType.
ArrayAttr would also not work “out of the box”, right now, the IR for ArrayAttr doesn’t have a type for the array. That can of course be fixed. But then the question is: are we ok with some type redundancy? E.g. would [1.0: f32, -1.0: f32] : complex<f32> be considered ok? If yes, then: problem solved
If not, the parsing would need to do something similar to TensorLiteralParser and temporarily store the tokens and parse them later once it knows the type of the elements. I don’t know in which contexts ArrayAttr is used, what kind of attribute elements can be inside it, and how much work would be involved to add parsing of one arbitrary attribute given a list of tokens.
I think you can just parse an Array attribute without passing a type, it will figure that the elements are floats and default to f64. We can declare the attribute as an F64ArrayAttr in ODS which will verify that all elements are indeed f64 and then have a custom verifier to make sure that there are only 2 elements.
I would opt for default printing and parsing, as well, as that maximizes reuse. Maybe this can even be specified via assemblyFormat in ODS.
Huh interesting, I didn’t realize that, but I see that the builder APIs etc don’t allow the type to be specified! The IR does track it though for all attributes, so it looks like it just can’t be set.
Yes, that sort of redundancy is inherent to how we model things as attributes, it occurs in other places as well. If you have these things occurring in some specific places (e.g. in the yourdialect.constant op) then you can sugar the syntax of that op of course. We have similar issues for even more basic cases, where you need a type for the attribute and also for the op.
-Chris
Yeah, we just never really needed to set its type so it was never exposed. There is nothing inherent here, it just doesn’t expose that ability in the current API.
The performance implication only really arise if you are trying to create a lot of them, e.g. if you were to use the Attribute iterator of a DenseElementsAttr(please anyone looking at this, you really shouldn’t be using that API 99% of the time). For simple “single” element attributes, the size factor really isn’t that much of a concern IMO.
– River
Ok, I have tried to adapt my prototype so that it uses ArrayAttr for complex constants. I added a builder that still accepts no type as parameter, that calls the (new) builder that expects a type but passes NoneType. For complex constants I would call the builder that expects a type and pass the complex type. But one issue I have run into is that if I change AttributeParser to parse an optional type after an ArrayAttr, we start parsing the following LLVMIR incorrectly:
%4 = llvm.insertvalue %2, %3[1 : i32] : !llvm.struct<(i32, !llvm.ptr<i32>)>
Right now, the parser expects that the type will be read as part of parsing the insertvalue op.
So either, we need to always print (and parse) a type for ArrayAttr, or we need another way how to know when to parse the type or not.
Any suggestions are welcome
Ok, I think I may have found a solution: we can add another builder to ConstantOp which allows to pass in the type in addition to the Attribute. This allows the FoldSplatConstants pass in FusionOnTensors.cpp to create a ConstantOp with a type which is the same as the element type of the DenseElementsAttr it tries to fold, without requiring to pass in the type via the Attribute. And then the print() and the parse() methods can be adjusted to print/parse a type if the attribute is ArrayAttr.
Here is the patch with this attempt: https://reviews.llvm.org/D101908
Coming back to this discussion. We now would like to add support for complex constants to the HLO constant operation. This requires to replicate all the special handling in printing/parsing to work around the fact that ArrayAttr does not have a type attached to its elements.
@clattner you were opposed to adding a ComplexAttr because it is not a built-in but a compound. It is not modeled that way currently, though. Likely, because there is no support for structs. I would agree with your argument if ComplexType was defined as a struct type. But until we have structs, I think having a ComplexAttr is the cleaner solution, to avoid littering downstream code with the special casing. If someone gets around to adding structs, we can reformulate the ComplexType that way and also redo ComplexAttr.
In summary: Adding a ComplexAttr does not really add a lot of code, is easy to refactor later and avoids special casing throughout the code base. So lets have it for now.
I don’t understand what you mean here, despite its name, ArrayAttr is a “struct” because it doesn’t require its elements to be homogenous.
The MLIR attribute hierarchy isn’t attempting to follow a type hierarchy, it is more closely to a JSON-like structured format. Needing to adding a first class ComplexAttr would imply that we would have to add a QuaternionAttr for more advanced cases - MLIR is intended to be scalable without this sort of core extension for domain specific use cases.
But it is, as a compound, essentially untyped. Having a StructAttr with a compound type that models what elements it contains would avoid a lot of special casing. When parsing, one could check against the expected struct type (or even require it) instead of having to do the dance of checking how many elements there are, whether each has the expected type, etc. It would also avoid the special casing around whether to print some invented compound type or not.
Agreed. However, Complex is a builtin type and I would like to have more structure when working with it. Would a StructAttr meet your bar for inclusion? It would require a StructType in builtin so that we can give it a type. Not to align the two hierarchies in general but to be able to support cases like complex.
Oh I see what you mean: You’re saying that the ArrayAttr itself doesn’t carry a type.
This isn’t inherent to ArrayAttr actually: it does carry a type (as does every Attribute), it is just that there is no way to specify it in the C++ API. I think it makes perfect sense for this to be settable, and have that round trip in the textual form. I think this would provide what you’re looking for, and eliminates the need for StructAttr.
-Chris
Regarding having a type for ArrayAttr: see my comment above from May 5th about the problem of parsing llvm.insertvalue
We could make ArrayAttr always print a type, I don’t remember whether I had tried that. But making it optional does not seem to work.
The problem is the round-trip in the IR. We cannot do this automatically as some current uses of ArrayAttr have a trailing type but the type does not belong to the ArrayAttr. @akuegel gave the example of llvm.insertvalue %2, %3[1 : i32] : !llvm.struct<(i32, !llvm.ptr<i32>)> where the : !llvm.struct<(i32, !llvm.ptr<i32> belongs to the op.
This means that for all typed uses of ArrayAttr, we need a custom parser to handle the trailing type. We could have an alternative syntax for ArrayAttr in the presence of the type, e.g. struct{1 : i32} : type. That would fix the ambiguity.
Doesn’t this instead mean that ops like insertvalue needs to have custom parsing even for the attribute, while the generic printing/parsing form of ArrayAttr should be typed?
You can also see it that way. In my view, we have different use cases here. ArrayAttr without a type is just a list of attributes. And I also want a variant where I can assign a type to the composite, which feels more like a struct.
I find it difficult to say which one should be the default.
This is a pragmatic problem, but isn’t a reason to add a new kind of attribute!
This argues that we need two syntactic productions for ArrayAttr in ODS: one that includes the type (or that optionally includes) and one that never does because it asserts that it is always missing. All the existing clients should use the later one.
I’m not sure the best way to handle this, but I’m saying that this is an ODS issue, not a C++ API issue. We could (for example) keep ArrayAttr in ODS to mean the “array attr with the invariant that the type is always missing”, and introduced a new TypedArrayAttr that would allow mlir::ArrayAttr’s that have a type.
-Chris
I am happy to add this. So we need a separate syntax for the typed version. Any suggestions? I proposed to use struct{...} : type but that admittedly was putting as little thought into it as possible.
Objections? Otherwise I will go ahead and add it.
No, the trailing type is already a part of every attributes syntax. I wouldn’t expect any special syntax. ArrayAttr moving forward should just elide the trailing type when it is UnitAttr.
– River
Can you elaborate on this?
In this example, the ArrayAttr has no trailing type but it is attributed to the op. The attribute parser never parses a type for ArrayAttr. So how can we have a generalized parser that can parse any attribute?