[RFC] Add a builtin ComplexAttr

I fear if we go through the ArrayAttr parsing, there will be problems with parsing the individual elements as the right type, unless each element has the type attached. The advantage of going through the TensorLiteralParser would be that it already knows how to store the tokens so that we can process them once we know the type of the elements. We could of course do the same for array parsing, but right now, it looks to me that ArrayAttr does not even have a type in IR format (instead it seems to rely on the attributes inside the array to have a type if needed).
So I guess if we want to store ComplexAttr densely, we would still want to reuse existing code for DenseElementsAttr? So we could set DenseElementsAttr as a base class, and use tensor<2xf32> as storage type. The AsmPrinter would however need to print it in a way so that the parser knows it is a ComplexAttr. If we use array syntax, we could change the ArrayAttr parsing logic to accept an optional type. And then either also print the type for each element in the array (e.g. [1.0 : f32, 2.0 : f32] : complex<f32>), or try to refactor TensorLiteralParser so that it can also be reused for ArrayAttr parsing?

What do you mean by this? What are you trying to achieve here?

I understand that the concern was that storing ComplexAttr as a std::pair<Attribute, Attribute> (as it was done in my prototype) has negative performance implications, and instead it should be stored densely, similar to how two values of the same type are stored in DenseElementsAttr. I am not attached to a particular idea or solution, I just want to get an agreement on a solution that works :slight_smile:
And from what I understand, technically it would be easiest to store a ComplexAttr the same way as a tensor<2xf32>. The difficulty with this is just that somehow we still need to be able to parse this and know that it is a ComplexAttr and not a regular DenseElementsAttr. Alternatively, I guess we could actually use DenseElementsAttr for complex constants (and not create a ComplexAttr), but let the constant have a complex type, and when printing the constant, we would print the complex type as well. Right now, it seems the type of the constant is only printed if the attribute is a SymbolRefAttr (printing the attribute from the constant would already print the type, anyway). So it would potentially look like this:
dense<1.0, -1.0> : tensor<2xf32> : complex<f32>
Which is kind of close to what you already suggested before (maybe it would be possible to adjust the printer and parser to avoid printing the tensor<2xf32> type, but it looks to me this could be tricky).
Maybe that solution isn’t actually so bad, even though it means that we would print complex values differently whether they are part of a complex tensor constant, or a complex scalar constant.

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?

-Chris

This is a fairly long discussion by now and at the risk of derailing it even further, here is my take.

DenseElementsAttr is designed for attributes that have a shape, so it does not really apply to complex scalars. The only commonality is that both have more than one element that make up their value.

We have specialized attributes for Float and Integer. Why should we not have one for Complex? I think the potential reuse of code here does not outweigh the added complexity of making parsing etc. work.

I think this might even be defined using TableGen. Or is that path discouraged?

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 :slight_smile:

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 :slight_smile:

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 :slight_smile:

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: ⚙ D101908 Add support for complex constants to MLIR core.

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?