This change adds a new DelinearizeIndexOp to the arith dialect. The
operation accepts an index type as well as a basis (array of index
values) representing how the index should be decomposed into a
multi-index. The decomposition obeys a canonical semantic that treats
the final basis element as "fastest varying" and the first basis element
as "slowest varying". A naive lowering of the operation using a sequence
of arith.divui and arith.remui operations is also given.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Generally looks good and is an important step forward.
I would either:
- move this op to arith and keep the operands as they are, or
- keep it in tensor but then take an actual tensor as the basis and abstract away the fact that you get the Dims as a lowering detail.
Unless you have a stronger argument for keeping the semantics as is that I don't see yet?
Rest of the impl LGTM so I'll approve and let you decide which way to go (or to justify the current form with an extra argument).
mlir/test/Dialect/Tensor/ops.mlir | ||
---|---|---|
270 ↗ | (On Diff #444426) | nit: nl here and below |
mlir/test/Dialect/Tensor/ops.mlir | ||
---|---|---|
270 ↗ | (On Diff #444426) | and above especially :p |
Move to Arithmetic dialect.
Diff is simplified because the lowering transform is moved to the existing -arith-expand-ops pass.
mlir/test/Dialect/Arithmetic/expand-ops.mlir | ||
---|---|---|
254 | nit: nl here and above. |
mlir/lib/Dialect/Arithmetic/Utils/Utils.cpp | ||
---|---|---|
131 | When does this happen and should the op be considered valid/pass verification where this is true? |
mlir/lib/Dialect/Arithmetic/Utils/Utils.cpp | ||
---|---|---|
131 | Is it possible to add a "non empty" constraint for the Variadic<Index> parameter via the Tablegen spec? If not I'll add a verifier |
mlir/include/mlir/Dialect/Arithmetic/IR/ArithmeticOps.td | ||
---|---|---|
1242 | is this small typo? -> %v1 = arith.muli %2, %3 : index |
This revision is really concerning to me because this operation is not close to any of the others in this dialect in terms of abstraction level.
mlir/include/mlir/Dialect/Arithmetic/IR/ArithmeticOps.td | ||
---|---|---|
1226 | This should be using ArithOp, also keep the operations in this file sorted as much as possible. | |
1230–1231 | Adding this operation to arith dialect is honestly significantky concerning to me, given that representationally this feels like a completely different level of abstraction compared to the rest of the dialect. |
Can you revert this and raise an RFC that adds additional context and justification? The direction this takes the arith dialect isn't clear to me.
I'm about to land a diff that depends on this: D129699 That diff is the main context.
Happy to move to another dialect that makes sense. Originally I created this diff to add the op to tensor, but was asked by @nicolasvasilache to move it to arith or change the spec:
Is your request to raise an RFC just about this op's positioning in the arith dialect? If so, I can revert and do option number 2 above to put the op back in Tensor, then no RFC needed?
The op from an abstraction point-of-view would make more sense in tensor, given that (at least at-present) the operation fundamentally breaks away from the purpose of the arith dialect (as defined e.g. by it's docs).
Going beyond that though, there is still a huge amount of context missing here and the documentation for the operation is quite lacking. Neither the operation documentation or the commit description really dig into the "why" behind this commit.
This should be using ArithOp, also keep the operations in this file sorted as much as possible.