This adds a tile_size parameter, when it is used the tiles are
cyclically distributed onto the threads of the scf.foreach_thread op.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp | ||
---|---|---|
1220–1222 | I recently learnt that there are good utils in https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Dialect/Utils/StaticValueUtils.h (maybe have to cast getTileSizes() to ArrayAttr.) | |
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp | ||
610–613 | We can use getAsValues(b, loc, nonZeroNumThreads) here. | |
621 | should we move the comment to l.604 or other places? :p | |
635 | let's rename it to initOperand because it matches DPS interface method more? | |
653–658 | I thought it's a no-op if tileSizes is empty. Please let me know if I'm wrong... If this is the case, can we do an early-exit at l. 587 -- when checking whether the tiling sizes are valid or not? | |
674–675 | can we rewrite llvm::seq to llvm:seq<unsigned>, so we can avoid casting? and maybe we can use structured binding for better readability, e.g., for (auto [index, result, bbArg] : llvm::zip(...)) { } | |
683–684 | They are only used in the for body, can we declare them with i and e? E.g., for (int64_t i = 0, e = ..., offIdx = 0, sizeIdx = 0 ...) |
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp | ||
---|---|---|
1220–1222 | I had tried it but I don't see how to make it work for int64_t in a less verbose way. (I don't think there is a way to cast ArrayRef<int64_t> to ArrayAttr. | |
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp | ||
621 | Thanks for catching that, I'm not sure how those comments got mixed up | |
653–658 | no it's not a no-op if tileSizes is empty then the problem is just divided amongst the given number of threads. | |
683–684 | it could but I personally find it makes readability harder. |
Address more review comments
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp | ||
---|---|---|
1220–1222 | Thanks for explaining in details Hanhan, fixed now. |
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp | ||
---|---|---|
659 | I think this is on a path to deprecation and should not be used anymore .. ? | |
665 | This is wrong, it should return the op, not the loop. @ftynse, we may have reached a point where we ned a sort of "runtime verifier" in the transform dialect to handle such issues? |
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp | ||
---|---|---|
1225 | unrelated to this PR but to be usable e2e the mapping needs to be passed, also adding that to my fixes. | |
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp | ||
671 | There are a bunch of bugs in this PR re insertion points, will also fix in my followup. |
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp | ||
---|---|---|
646 | this is an op modification that does not go through the rewriter .. |
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp | ||
---|---|---|
646 | I'm always confused how those can go through rewriter? Are we supposed to loop through the users and do inplace root update? I don't see if being done in other rewrite patterns. | |
659 | I think IREE doesn't rely on it anymore. Are the foreach_thread tiling also going to move into the same file as the other patterns using TilingInterface? Otherwise it makes re-using helpers hard. |
I recently learnt that there are good utils in https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Dialect/Utils/StaticValueUtils.h
Can we rewrite it to SmallVector<opFoldResult> tileSizesResult = getAsOpFoldResult(getTileSizes())?
(maybe have to cast getTileSizes() to ArrayAttr.)