This cleans up a lot of parameter passing. It also prepares adding
proper "delegate" functions to the new environment and moving this
out into its own class with a better OO design.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
At a first glance. This definitely is a good start!
But I also feel many variable should be private. (e.g., topSort maybe also merger). And we have more meaningful APIs for things like env.topSort.size().
Maybe this is also what you mean by better OO design and more delegate functions?
I will take a closer look next week.
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
98 | Would it be better if loop emitter is owned by Env? Maybe you want a emitter.initialize API from me to make it work? If so, Just let me know! |
Indeed! This is just the very first mechanical step. After this ,the next thing is to provide the right API with delegate functions. So instead of env.merger.foo(), we get env.foo() and then under water this goes to merger, or perhaps emitter. Or perhaps we want another design, e.g. env keeps some data that should be neither in merger nor in emitter. But using this delegator class, it will become much easier, hopefully!
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
98 | Yes! That is what I meant with this TODO and more importantly the one at L1703. Right now we cannot construct everything at once, since loop emitter constructor needs contents of topsort. So that is why we need to wait until line 1703 until we can set it up. And having an initializer or start was indeed my next request to you, so we can make that happen. |
The overall direction looks nice. However, I think most/all the functions which take CodeGenEnv& should instead be made into methods. That'll help make the function/method parameter lists shorter, and help reduce the size of the diff somewhat (since it avoids needing to add the env. qualifier everywhere).
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
56 | "structures" | |
69 | Maybe should add assert(!loopEmitter) just to be safe? (Unless overwriting old loopEmitter is the intended design) | |
78 | Since there are a number of places that want to do this std::vector to ArrayRef conversion, it might make sense to add a method ArrayRef getTopSort() const. And then this method's body becomes just return getTopSort().slice(...); Of course, since a lot of places that do the std::vector to ArrayRef conversion immediately follow it up with a slice, it may make sense to define a getTopSortSlice method too/instead. | |
82 | llvm-style nit: should save the size in a temp variable rather than calling the method in the condition. (I'm pretty sure https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop still applies, even though it's the size method rather than end.) | |
108 | MLIR code elsewhere uses var{nullptr}; syntax for this situation. | |
193 | (See my previous comment re getTopSort/getTopSortSlice methods) |
Introduced delegates for the most frequently occuring calls to merger/emitter.
Note that an exhaustive cleanup + move of all member fields to private is planned
when this struct is moved out into its own file, and emitter init support is added.
Note that in the next revision, I plan to move this struct out completely, and provide access through proper methods only (which will get rid of all the env.xxx.yyy() constructs.
However, I kept the changes in this first step smaller on purpose, so that it is easier to review the initial parameter change, before doing that deeper refactoring.
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
69 | Good idea. Note that the next change will be to have emitter as field here, but right now the constructor does not allow such a setup.... | |
82 | Oh agreed, and I prefer that style too. But note that it was pre-existing. |
"structures"