This revision extends the sparse compiler support from fp/int addition and multiplication to fp/int negation and subtraction, thereby increasing the scope of sparse kernels that can be compiled.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Initial set of comments. Not yet done with review, but I figured I should give you some comments to read through!
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
127 | ||
127–128 | I'm not sure I understand the name and doc comment for this function, though that's likely just because I've never heard the term "identity merge" before. It seems like this function takes a set of points and creates a new set of points, with the expression at each new point having type kind and having the corresponding point from s as its only child. Is that correct? | |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
639–647 | I understand what's happening here, but why isn't there an equivalent NegIOp? | |
mlir/lib/Dialect/SparseTensor/Utils/Merger.cpp | ||
26–31 | This may matter less after the union-refactoring, but why did you choose to store the negation's child expression in e1 and not e0? | |
102–104 | should there be some assertion here that kind == kNegI || kin == kNegF? It seems like that is an implicit assumption of this function. | |
199 | It seems like this function is only meant to be used for binary ops. If that's the case, could you just assert that kind is a binary operator kind? Additionally, maybe it should be called something like kindToOpSymbol? | |
304 | I'm unsure what this comment means! (Perhaps related to the fact that I don't understand the use of "Id" in the function name, related to the comment above) |
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
127–128 | Exactly. The identity applies to the lattices themselves, but it adds an operator to the expression. | |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
639–647 | The short answer: just to keep code concise and consistent for binary ops only. Slightly longer: | |
mlir/lib/Dialect/SparseTensor/Utils/Merger.cpp | ||
26–31 | Yeah, this code is gone, but I did it so that 0-y and -y are "the same" ;-) | |
102–104 | Agreed. Added! |
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
127–128 | To me it feels almost like a map operator in functional programming. You're mapping an expression constructor over a lattice set, to produce a new lattice set with the given expression type. So perhaps something involving map? Alternatively, if this is only meant to work with unary ops (or even just negations), you could make it even more specific (e.g. constructNegationsFromSet or mapUnaryOpOverSet) | |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
639–647 | Interesting that std does not support NegI. Makes sense! |
mlir/lib/Dialect/SparseTensor/Utils/Merger.cpp | ||
---|---|---|
351 | It seems like this function just fails silently/returns None at the moment, e.g. if we encounter an op with one operand that doesn't produce a value on this line. Is that expected behavior? |
Finished reading through everything. Just have some questions, but for the most part everything looks good!
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
127–128 | Since we will (may) use this in the future for other ops too (division for example), and its main goal is to introduce a zero at places where we cannot omit the op (e.g. 0+a = a but 0-a must keep it), how about simply mapZero()? | |
mlir/lib/Dialect/SparseTensor/Utils/Merger.cpp | ||
351 | Yes, anything that does not build (also unknown binary ops by the way) falls into None Then in Sparsification we have // Builds the tensor expression for the Linalg operation in SSA form. Optional<unsigned> exp = merger.buildTensorExpFromLinalg(op); if (!exp.hasValue()) return failure(); |