Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Unit tests: pass. 62304 tests passed, 0 failed and 838 were skipped.
clang-tidy: pass.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
What happens when you have non-parallel loops in the generic op? Can we have a test for this case (even if there's no transform happening, just make sure we bail out instead of generating incorrectly parallel loop?)
mlir/include/mlir/Dialect/Linalg/Passes.h | ||
---|---|---|
38 | Given that we have parallel loops to sequential loops, does it make sense to keep Linalg to sequential loops? | |
mlir/include/mlir/Dialect/LoopOps/LoopOps.td | ||
179–186 | Any specific reason for this rename? I find "body" to be a better name than 'region". | |
mlir/lib/Dialect/LoopOps/LoopOps.cpp | ||
235 | Ultra-nit: drop trivial braces |
mlir/include/mlir/Dialect/Linalg/Passes.h | ||
---|---|---|
38 | Yes, it does. We want to go LHLO->Linalg->Ploops->GPU. | |
mlir/include/mlir/Dialect/LoopOps/LoopOps.td | ||
179–186 | This is what we have in loop::ForOp. Just wanted to be consistent. I can change both though. I can also change getBody()->body() in ForOp and in ParallelOp. |
Thank you for the comments. I added a test and a check, that should fail if we are lowering to ploops with any non-zero number of window/reduce loops.
Unit tests: fail. 62336 tests passed, 1 failed and 838 were skipped.
failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp
clang-tidy: pass.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
mlir/include/mlir/Dialect/Linalg/Passes.h | ||
---|---|---|
38 | Why? How having Linalg->Loops helps you? You are going to have LHLO->Linalg->Ploops->GPU, and you can also have LHLO->Linalg->Ploops->Loops->e.g.CPU because we have Ploops->Loops. I don't see the need for Linalg->Loops in this scheme, it will be mostly duplicate code. | |
mlir/include/mlir/Dialect/LoopOps/LoopOps.td | ||
179–186 | Consistency is good. Change it in a follow-up if you want. | |
mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp | ||
416 | Nit: MLIR uses lowerCamelCase for function names | |
mlir/test/Dialect/Linalg/parallel_loops.mlir | ||
2 | Do we need really need --dump-input=always ? |
mlir/include/mlir/Dialect/Linalg/EDSC/Builders.h | ||
---|---|---|
80 | nit: conditional_t here and below | |
mlir/include/mlir/Dialect/LoopOps/LoopOps.h | ||
47 | /// | |
mlir/include/mlir/Dialect/LoopOps/LoopOps.td | ||
184 | Is this wrapped at 80? | |
mlir/include/mlir/EDSC/Builders.h | ||
224 | Can you add a comment to this? | |
mlir/lib/Dialect/Linalg/EDSC/Builders.cpp | ||
119 | nit: lbs, ubs, steps? | |
mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp | ||
606 | /// | |
mlir/lib/Dialect/LoopOps/LoopOps.cpp | ||
351 | dyn_cast? | |
mlir/lib/EDSC/Builders.cpp | ||
196 | llvm::to_vector<4>(lbHandles|ubHandles|steps) | |
202 | *Drop trivial braces | |
mlir/test/Dialect/Linalg/parallel_loops.mlir | ||
2 | dump-input-on-failure is a better choice here. |
Unit tests: fail. 62344 tests passed, 1 failed and 839 were skipped.
failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp
clang-tidy: pass.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
Whoohoo. \0/ Thanks!
mlir/include/mlir/Dialect/Linalg/Passes.h | ||
---|---|---|
38 | I think you are cross-talking here. What ftynse suggests is always going LinAlg->Ploops->SeqLoops->... instead of also having LinAlg->SeqLoops. That is a good point. I would suggest we build out LinAlg->Ploop first before retiring the other lowering, though. | |
mlir/include/mlir/Dialect/LoopOps/LoopOps.td | ||
179–186 | Just adding my 2 <insert small change of currency of choice> here. I find the name body misleading, especially when there also is a getBody() method on the same op. Hence I prefer region for the region and body to return the first block (if there is a single block in the region). | |
mlir/lib/Dialect/LoopOps/LoopOps.cpp | ||
234 | ParallelOp::ensureTerminator | |
329 | I just saw it is wrong here, too :( |
Coming a little late to the party but this looks great, thanks Alex-DE!
+1 to Alex-FR's suggestion of Linalg -> ploop -> loop when it makes sense, and similarly in the future Linalg -> affine.ploop -> affine.loop.
Hi @pifon2a, I noticed that your work appeared on GitHub at remotes/llvm/cl291881283. Please stage work on your fork in the future, I think those branches are just for releases. Thanks.
nit: conditional_t here and below