Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LG but please run clang-format.
mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeTransposeConv.cpp | ||
---|---|---|
376–379 | Please install clang-format on your machine, arc will run it when you upload patches. |
Apply clang-format to: mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeTransposeConv.cpp
mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeTransposeConv.cpp | ||
---|---|---|
376–379 | I have no idea how this got through with arc as I had clang-format on my machine. Will pay more attention next time. |
mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeTransposeConv.cpp | ||
---|---|---|
39 | I am using clang-format version 10.0.0-4ubuntu1~18.04.2. I'm not its why these are showing up. |
mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeTransposeConv.cpp | ||
---|---|---|
39 | In general we're using the last LLVM release, but clang-10 should catch these :) |
Please update TosaToLinalg lowerings to include transpose decomposition:
mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeConv2D.cpp | ||
---|---|---|
2 | This line is incorrect, please update. | |
10 | Please detail what it is replaced by. | |
21 | Please remove unused dependencies. I am quite certain BlockAndValueMapping is not being used. | |
mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeDepthwise.cpp | ||
1 | Please fix. Also, rename file something more specific. TosaDepthwiseToMul would be more apt. | |
9 | Should include what the pattern decomposes into. | |
mlir/test/lib/Dialect/Tosa/TosaTestDecomposePass.cpp | ||
10 | Detail more than helper functions. Overly vague. |
mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeDepthwise.cpp | ||
---|---|---|
1 | I am not 100% sure about renaming. Creating a file per specific rewrite seems unscalable. This file should be the location we place where we put any Depth wise decomposition rewrites. Just like the TransposeConv has multiple rewrites in the future we may have multiple rewrites here. |
Some minor changes on my end. Waiting on @mehdi_amini to review.
mlir/include/mlir/Conversion/Passes.td | ||
---|---|---|
652 | Rewrites is superfluous as the majority of passes rewrite IR. I suggest something such as tosa-optional-decompositions. | |
mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeDepthwise.cpp | ||
1 | I agree this is an issue. I would have preferred having one TosaOptionalRewriters file that have groupings of useful rewriters. In this case, the file name and comment header did not match, which was the primary issue. | |
mlir/test/lib/Dialect/Tosa/TosaTestDecomposePass.cpp | ||
2 | Should match filename. |
It's not clear to me why this is in Conversion when it seems to me a transformation that operates TOSA->TOSA?
Conversion/XXX directories are really intended to be trimmed to the conversion code that spans two dialects (or more).
I believe we agreed that we would expose these optional rewrites as populate() functions within the TOSA Dialect folder and then allow the consumer to add them to the their rewrite engines. Otherwise we would need to create a pass in the TOSA folder which would take us back to the original solution this patch is trying to refactor.
What I'm saying is that I'm not sure why the TosaOptionalDecompositions pass is not just defined in the Transforms directory?
Also, why do you have TosaTestDecompose if you need anyway a public TosaOptionalDecompositions pass? I can't spot the difference between the two right now.
I understand your point we have ended with similar passes in the test/ and linalg/ folders. Given that I have reverted to a single pass in the tosa/transforms folder but with naming and organisation fixed as previously discussed.
Rewrites is superfluous as the majority of passes rewrite IR. I suggest something such as tosa-optional-decompositions.