works both along runtime path and pure codegen path
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
---|---|---|
760 | Is there any better way to abstract over this? (to avoid magic numbers / ensure it stays in sync with the struct definition) | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
952–956 | Should factor out a genSparseValuesCall function, to deduplicate this code and the version in SparseTensorToValuesConverter::matchAndRewrite |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
---|---|---|
760 | Yeah, that is a good suggestion, since the -2 has bitten me before. Added a general util for MemSizes field | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
952–956 | That is really just two lines of code and will break the nice symmetry with the other "getters". |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
---|---|---|
952–956 | The biggest reason to share it is to avoid the "magic constants" problem re the 15 and "sparseValues". Most of the other genFooCall functions are similarly short, but exist to address the same maintenance concerns. Perhaps there won't be any other places that use it in the future, but whenever I see the same magic constants more than once that's a warning flag for me, especially when they're strings :) I don't quite follow what you mean about symmetry with other getters, but this would give symmetry with the other genFooCall functions. |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
---|---|---|
952–956 | Ok (by symmetric I meant that all SparseTensorToXXXConverters looked very similar and this would break that, but not a big deal). The <15> is a good point [is there a templating way of getting rid of the need for that, btw, it seems we could strlen somehow at compile-time?! |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
---|---|---|
952–956 |
Gotcha. Yeah, when looking over the code a while back for the Enums refactoring, I saw a few places where the setup for createFuncCall wasn't abstracted into a genFooCall wrapper; but since they were the single place the given function was called, I didn't bother writing up a CL to add the wrappers (because definitely not worth it if there's only one callsite :)
It's doable, but I'm not sure it's worth the code it'd take to do it. Last time I thought about it, the simplest way I could come up with was to store the base-name string literal as a global constexpr, and then have some macro for threading that constexpr into both the size and the value; i.e., something like: static constexpr const char *kFooBaseFunctionName = "foo"; static constexpr unsigned kMaxLenPrimaryTypeFunctionSuffix = 3; ... # define PRIMARYTYPE_FUNCTION_NAME(VAR, BASE, ETP) \ SmallString<std::strlen(BASE) + kMaxLenPrimaryTypeFunctionSuffix> VAR{BASE, primaryTypeFunctionSuffix(ETP)}; ... static Value genFooCall(...) { PRIMARYTYPE_FUNCTION_NAME(name, kFooBaseFunctionName, elemTp) ... } Of course we'd also need to define our own version of strlen since std::strlen isn't constexpr. (But that's easy enough to do, since this is compile-time and the strings are short, therefore performance isn't much of a concern.) We could probably convert the macro into a proper template by using tricks similar to https://en.cppreference.com/w/cpp/types/integral_constant; i.e., so we could define the moral equivalent of: template<string BaseName> class PrimaryTypeFunctionName { SmallString<strlen(BaseName) + kMaxLenPrimaryTypeFunctionSuffix> name; public: PrimaryTypeFunctionName(Type elemTp) : name(BaseName, primaryTypeFunctionSuffix(elemTp)) {} StringRef getName() const { return name; } } Then we could just use it à la PrimaryTypeFunctionName<"foo"> name(elemTp);. Though I'm not sure how close to this ideal we could really get. Afterall, the whole point of using SmallString was to allocate these small buffers directly on the stack; so we wouldn't want to break that. There might be something else in the STL (or LLVM/MLIR utils) which can help simplify this metaprogramming, but I didn't see anything last time I thought about all this. |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
---|---|---|
952–956 |
Neat! Thanks for figuring that out. I agree with you that this is probably not worth the extra machinery, given that we only have a handful of such methods. |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
---|---|---|
934 | Neat, I was worrying whether there is an implicit copy here, but seems runtime library just copy the pointer ;-) |
Is there any better way to abstract over this? (to avoid magic numbers / ensure it stays in sync with the struct definition)