Does anyone have any thoughts on relaxing the type constraints for a FloatAttr to allow it to use LLVM::LLVMType types (that correspond to floating point) as well as the standard FloatType?
There is a quirk in an LLVM::ConstantOp in that it takes an Attribute for its value. That Attribute, in our case, is a FloatAttr with a FloatType, even though we are trying to emit an LLVM constant with a FP semantics distinct from the standard dialect supported ones.
An alternative approach might be to teach the LLVM IR dialect converter to call out to our code when it finds a custom dialect attribute rather than a standard attribute.
–
Eric
Adding more description:
X is an LLVM::ConstantOp containing:
attribute A which is a FloatAttr containing:
value V which is an APFloat with semantics double-double
type FT which is a (standard) FloatType of <no standard FloatType for double-double>
So this is not quite ok in some ways, but fails various type checks for obvious reasons.
FloatAttr is part of the “core IR” and a lot of things depend on it, so I would prefer to keep it independent of the LLVM dialect. Practically, making FloatAttr aware of LLVMType would create a cyclic dependency between MLIRIR and MLIRLLVMIR targets, and carry a lot of LLVM-specific baggage into every user of MLIRIR.
An alternative approach might be to teach the LLVM IR dialect converter to call out to our code when it finds a custom dialect attribute rather than a standard attribute.
You can restrict the existing pattern to fail the match if the value attribute is of the type it cannot recognize (which is the right thing to do anyway in case it doesn’t do it). Then you can have a pattern that matches std.constant with value attribute of your type and does the conversion the way you want, and plug this pattern in your lowering process.
Another possibility is to introduce LLVM-specific attribute types that support the cases missing from the standard attribute. The conversion will then also recreate the attribute instead of carrying over the standard ones.
Could you please elaborate what is missing from the standard FloatAttr for you? Maybe we should just fix it instead. I vaguely remember debugging some semantic discrepancies between APFloat and FloatType in the past.
would need to know what the type is to be able to construct the correct APFloat, but these should be fine:
/// Return a float attribute for the specified value in the specified type.
static FloatAttr get(Type type, const APFloat &value);
static FloatAttr getChecked(Type type, const APFloat &value, Location loc);
If the former APIs are needed, perhaps they should be renamed to getStd(type, value) instead of get to make it clear they only work with standard types?
The problem is that there are a lot of interconnections within attributes that need to know specific details about the types of them, e.g. we would now need additional verification in the other attribute users of FloatAttr that the type is something expected. That leads to a very bad experience, at least IMO. One example is that in the ElementsAttr attributes, we store raw data and rely on the type to reconstruct something that can be manipulated by users. If we are going to open up these storage types to dialect defined types there has to be some way (e.g. like interfaces for types @jpienaar ) that can provide us with the information we need to know. To cover the current attributes we would need to have some interface like thing for integer-like, float-like, and shaped-like types.
Could you please elaborate what is missing from the standard FloatAttr for you?
The crux of the problem is the dependence from the LLVM IR dialect’s constant (operation) on standard MLIR types. These MLIR types are closed and incapable of representing dialect Foo’s constants, attributes, or types. Since the goal is to lower the Foo dialect to LLVM via the LLVMIR dialect bridge, it means there is a missing span in that bridge.
For instance, in my dialect, I might write something like
This is a fp constant that cannot be represented with std.constant nor use a FloatAttr. That’s ok, since we clearly don’t.
However, we still very much want to lower this correctly to LLVM as
... x86_80 1.986440e-25 ...
We can already, of course, carry this value around in an APFloat.
What I’m grappling with is what is the most desirable way to go about adding the missing span to the LLVMIR dialect bridge. When converting to LLVMIR dialect, we necessarily lose Foo’s dialect operations. (I don’t think we want to go down the road of adding more standard fp types—e.g. f80, f128 and df64 or ?—or allowing custom extensions to LLVMIR dialect operations either.) So the obvious choices are adding the smarts with the attribute or with the type hanging off the attribute so that the APFloat value contained inside is correctly recognized and constructed in the final LLVM code. The LLVMIR dialect already knows about the LLVM fp types. We just need a viable way to get there.
Perhaps, as River suggests, type interfaces is the way to go and extending !fir.real in such a way that the LLVMIR dialect does the proper conversion to LLVM. But there are other possibilities as well.
Type interfaces sounds like something quite useful! On the SPIR-V side we would like to reuse DenseElementsAttr for nested array types but right now it is only accepting standard vector/tensor types so ends up we are having inconsistency between the attribute’s type and the spv.constant’s type.
Would it be possible to shift our philosophy on these standard attributes a bit? With FloatAttr as an example, we could say that it models an APFloat, but the type assigned to the attribute may be anything. It would be up to the operation to ensure that the type made sense. For example, it would then be legal to have an “f80 1.0” FloatAttr, but with a dialect specific type - this would just be assumed to be valid.
We can do a bit better for the standard types, and do checking of them (to make sure an f80 constant doesn’t have bf16 type), but this seems like a nice to have, not something core to the behavior of the type.
Most attributes (except ArrayAttr and StringAttr) are typed. We had this discussion several times, and although we never reached or formalized the conclusion, we leaned towards having all attributes typed.
We could think of deriving the type of FloatAttr from the floating point semantics of the underlying APFloat, but we would still need some consistency guards, e.g. what does mean to have %0 = constant APFloatValue(minExponent=-1, maxExponent=42, precision=7, bits=17) : f32?
When writing my initial answer, I considered suggesting an extension to the constant hooks that we have for ElementsAttr in order to support scalar types in addition to tensor types, but then decided that having a different lowering/attribute is just simpler.
Then let’s address exactly that. LLVM IR dialect’s constant should not use FloatAttr, but rather have an LLVM IR dialect attribute #llvm.fp for representing all floating point constants representable in LLVM IR. Then both std.constant(FloatAttr) and fir.constant(#fir.real) can convert to that.
I’ve made an opportunistic choice of reusing FloatAttr back in the day because the standard dialect was the only thing that was lowering to LLVM. It’s a limitation of the current scheme, not a hard or necessary constraint. FWIW, I disliked the fact that the LLVM dialect depends on standard attributes, but I disliked extending extra effort without a use case even more
I’m sorry, but I still don’t understand this design point. There are two different things that FloatAttr can be (keeping in mind that all dialects can define their own attributes, so we’re just talking about the generic thing called FloatAttr):
It could be an attribute that has one of the “standard” floating point types, and have semantics tied to the type instead of the representation.
It could be an APFloat that has any MLIR type associated with it, and have semantics disassociated from the type.
Why is #1 better? It seems useful for something like FloatAttr (and IntegerAttr) to contain an APInt and APFloat and be disassociated with their underlying type. This is the same policy we have for arrays and other things, where we don’t force them to have a standard type. Such an approach is strictly more general and useful, which seems like a good thing for “standard attributes” that value extreme reuse across dialects.
Allowing just any type seems a bit dangerous to me. We could end up with FloatAttr having tensor<*x!linalg.range> as type, and I don’t see what semantics that could have. As one example, we can see std.constant as a cast from the type of FloatAttr to the result type of the operation, at which point allowing any type on the attribute is similar to having std.cast from any type, although for constants only. In another thread, we decided that it wasn’t a direction we want to take.
We need some place where the compatibility between the type of FloatAttr and the semantics of the APFloat will be stated. If it is not the attribute itself, it will be spread (potentially with repetition) across all users of the attribute. If we had a way of only allowing “float-like” types, it would sound more reasonable.
We might just remove the type from FloatAttr completely, keeping it only a wrapper around APFloat. The users of the attribute will then decide which type they want to associate with it and what are the compatibility rules.
ArrayAttr always has NoneType, so in a sense its type is ignored in the IR. It is practically impossible to assign a “wrong” type to it, but it would become possible for FloatAttr should we allow any type. I just don’t see the point of storing an additional type in the attribute, which may diverge from the APFloat semantics in ways that cannot be trivially checked by the infra (because types can be dialect-specific, and we don’t have a dialect-agnostic way of saying “this type has this FP semantics”). Hence my comment:
We might just remove the type from FloatAttr completely, keeping it only a wrapper around APFloat . The users of the attribute will then decide which type they want to associate with it and what are the compatibility rules.
FWIW, I think the original thinking was that all attributes should be typeless. But it hit limitations in roundtripping things like ElementsAttr where you’d need to know the size and the memory layout of each element (essentially its type) to be able to interpret the contents of a container for printing/parsing/indexing.
) that can provide us with the information we need to know. To cover the current attributes we would need to have some interface like thing for integer-like, float-like, and shaped-like types.