Differential D101008
[mlir][tensors] Introduce attribute interface/attribute for tensor encoding Authored by aartbik on Apr 21 2021, 6:53 PM.
Details The new "encoding" field in tensor types so far had no meaning. This revision introduces:
Active discussion:
Diff Detail
Event Timeline
Comment Actions I am now comfortable enough with the attribute interfaces PTAL
Comment Actions I still would rather see this as just attributes on the SparseTensorAttr class, until there is a demonstrated need to make this an interface (and that this particular interface is the right choice). But I don't have the energy to argue it further. Carry on. Comment Actions It is not my intention to wear you out ;-), just to arrive at the right design. I was struggling a bit myself to get this right, so I may have gone back and forth too many times. Please give the latest one more careful look.
VerifiableTensorEncoding : an API for verifying that encoding matches tensor type data used by parser/verifier for "single" point verification
Comment Actions I trust you that it has the right components. But even assuming that:
Note that there is nothing second-class about #tensor.sparse. Having something in the IR directory doesn't really confer any blessed benefit (and we could move the SparseTensorEncodingAttr there if there was any). In fact, I think that having #tensor.sparse in the tensor dialect, and not needing to touch the IR/ directory, is a validation of the effectiveness of our "encoding" design choice -- the whole point was to make this something that an open-ended ecosystem could be built out around, *without* touching IR/ or an interface directory that everything else depends on. Indeed, that was the entire goal of the "encoding" design in the first place. Comment Actions Thanks Sean. At some point it of course becomes a bit subjective, but since we already need to add the VerifiableTensorEncoding attribute interface to "IR" to ensure single-point verification of all future encodings, it seemed right to bundle all abstract API's for the encoding, such as the conceptually agreed upon SparseTensorEncoding API, at the same place. And indeed other groups already have other attributes for TACO style encoding, and they could migrate simply by implementing the interface (although, of course, arguably, they could also just write an attribute converter). We don't have to decide today. Let's see if others have a preference one way or the other.... Comment Actions I think Sean is disputing that the sparse API is an abstract one actually. The VerifiableTensorEncoding is non disputed as it applies to every possible encodings attribute.
That's interesting: could they just use the concrete SparseEncoding attribute or do they store different kind of information on top of what you store here? (which would motivate the interface as an extensibility point). Comment Actions From the slides, the Comet project has tensor algebra operations like the one below. Even though it looks like an ordinary dictionary attribute is used, it also looks like an attribute that is sufficiently complex that they want to define it in a dialect sometimes, while implementing the SparseTensorEncoding API to connect to MLIR backends. I have pinged Gokcen to see what she thinks. "ta.tc"(%1,%2,%3){ ... other info ...., f64, formats=["Dense", "Dense", "Dense"], indexing_maps = [ ... affine maps ... ] } : tensor<32x32x32,f64>, ...Comment Actions after discussion offline: (1) keep attribute interface for verification in IR
Comment Actions LGTM from me. Please get LGTM from someone with sparse compiler expertise on the exact API for SparseTensorEncodingAttr (choice of default, what would be most ergonomic, etc.).
Comment Actions (Not a compiler expert.)
Comment Actions Sorry, I did not see the follow up discussion until you pinged me. I indeed picked a dictionary since it seems easiest for parsing/building (since fields are optional, at first glance making custom parser/builders seemed a lot of work to deal with absence/presence, arbitrary order etc.) with indeed just slightly more code in verification on making sure key-values are as expected. This way, I can also extend the attribute with a new key-value pair easily by just adding a few lines in the verifier + API, the parser/builders remain untouched. Lastly, I like the syntax of having named fields in the "struct " ;-) #CSR = #tensor.sparse< { sparseDimLevelType = [ "dense", "compressed" ], sparseDimOrdering = affine_map<(i,j) -> (i,j)>, sparsePointerBitWidth = 64, sparseIndexBitWidth = 64 }> I may be wrong there simply not having written many custom attributes before. Perhaps all this (named values, optional values, arbitrary order etc.) is just a easy there. But, genuinely curious, does it matter a great deal? The parsing overhead and memory footprint of a sparse tensor type does not really seem to be on any critical path. But as always, happy to rewrite to a struct-like attribute if there is a strong push, but I hope that my arguments above at least make some sense. (and sorry for not responding to post-commit earlier) Comment Actions Not that I agree with you here, but it does not matter: none of these I believe would impact the attribute definition: you could still parse just a dictionary attribute without storing a dictionary in the attribute itself (i.e. parse a dictionary, and then extract the information to build the attribute itself)
I don't really mind the parsing, I mind the API that will be exposed in C++ to all the user of this class: I don't want to see a builder for this attribute based on a Dictionary for example, neither the storage should be based on this. Comment Actions That is actually a strong argument, especially since we want this type to be easily generated by front-ends eventually too. We will, of course, still cross our swords, purely metaphorically, on the use of an affine map ;-) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
It isn't clear to me that we need a dictionary here.
Why can't we explicitly list the members instead?
The whole implementation is pretty noisy and there are too many unnecessary indirection because of this dictionary. This is akin implementing a regular C struct with some sort of map<string, value> instead.