The implementation of fusion on tensor was initially planned for just
GenericOps (and maybe IndexedGenericOps). With addition of
linalg.tensor_reshape, and potentially other such non-structured ops,
refactor the existing implementation to allow easier specification of
fusion between different linalg operations on tensors.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Mostly looks good but the extra level of indirection through the template class was harder to follow than necessary I think.
Can we make it just an untyped entry point and drop the extra class ?
mlir/include/mlir/Dialect/Linalg/Utils/Utils.h | ||
---|---|---|
77 | This looks like an easy way to get link errors. | |
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp | ||
415 | This level of indirection does not seem to pay for itself. if (auto genericOp = dyn_cast<GenericOp>(producer)) { on l. 590 |
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp | ||
---|---|---|
415 | I want to keep the implementation of different fusions separate. This only has fusion of GenericOp with GenericOp. The follow up CL just adds the method to fuse GenericOp with TensorRehapeOp, and TensorReshapeOp with GenericOp. (see D78464) |
Addressing comments
Updating D78463: [mlir][Linalg] NFC: Refactor fusion on tensors to enable extending it
to fusing different kinds of linalg operations on tensors.
Remmoving indirection in class heirarchy.
Updating D78463: [mlir][Linalg] NFC: Refactor fusion on tensors to enable extending it
to fusing different kinds of linalg operations on tensors.
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp | ||
---|---|---|
415 | Removed the level of indirection here. PTAL |
What is the plan for the fusion in general? Is this intended to stay specific to Linalg? Is this supposed to be extensible composable? It does not seem to be designed with interfaces to abstract away the dialect operations right now (the path under lib/Dialect/Linalg/Transforms/ may also contribute there).
There is an XLA fusion pass getting tested in the TensorFlow repo but that is a good candidate for upstream at some point. However we need to figure a path for all of these to compose.
This looks like an easy way to get link errors.
Can you avoid templating here, just take Operation* as a consumer and do a switch underneath?