Page MenuHomePhabricator

Summary: [mlir][tosa] Move all TOSA Operations Decomposition Patterns into populatePatterns() functions, that can be used by external rewrite engines
ClosedPublic

Authored by aardeb on Jan 3 2022, 1:28 AM.

Diff Detail

Event Timeline

aardeb created this revision.Jan 3 2022, 1:28 AM
aardeb requested review of this revision.Jan 3 2022, 1:28 AM

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.
I also recommend setting up the pre-push hook: https://llvm.org/docs/GettingStarted.html#git-pre-push-hook

aardeb updated this revision to Diff 397153.Jan 3 2022, 3:18 PM

Apply clang-format to: mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeTransposeConv.cpp

aardeb marked an inline comment as done.Jan 3 2022, 3:25 PM
aardeb added inline comments.
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.

aardeb marked an inline comment as done.Jan 3 2022, 3:29 PM
aardeb added inline comments.
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.

aardeb updated this revision to Diff 397155.Jan 3 2022, 3:31 PM

Fix Lint Pre-merge checks

mehdi_amini added inline comments.Jan 3 2022, 8:46 PM
mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeTransposeConv.cpp
39

In general we're using the last LLVM release, but clang-10 should catch these :)

rsuderman requested changes to this revision.Jan 4 2022, 12:08 PM

Please update TosaToLinalg lowerings to include transpose decomposition:

https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgPass.cpp#L70

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
9 ↗(On Diff #397155)

Detail more than helper functions. Overly vague.

This revision now requires changes to proceed.Jan 4 2022, 12:08 PM
aardeb updated this revision to Diff 397537.Jan 5 2022, 5:02 AM
aardeb marked 6 inline comments as done.

Fix Comments and Update TosatoLinalg Conversion

aardeb added inline comments.Jan 5 2022, 5:04 AM
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.

aardeb updated this revision to Diff 397544.Jan 5 2022, 5:27 AM

Fix Linting Issue

Some minor changes on my end. Waiting on @mehdi_amini to review.

mlir/include/mlir/Conversion/Passes.td
652 ↗(On Diff #397544)

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
1 ↗(On Diff #397544)

Should match filename.

mehdi_amini added a comment.EditedJan 5 2022, 7:35 PM

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).

aardeb updated this revision to Diff 397848.Jan 6 2022, 4:02 AM

Fix Naming issues

aardeb marked 2 inline comments as done.Jan 6 2022, 4:04 AM
aardeb updated this revision to Diff 397849.Jan 6 2022, 4:08 AM

Fix Spelling mistake

aardeb updated this revision to Diff 397852.Jan 6 2022, 4:13 AM

Fix comment

aardeb added a comment.Jan 6 2022, 4:18 AM

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.

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.

aardeb updated this revision to Diff 398539.Jan 10 2022, 2:03 AM

Remove multiple similar passes

aardeb updated this revision to Diff 398540.Jan 10 2022, 2:06 AM

Fix Cmake File

aardeb updated this revision to Diff 398541.Jan 10 2022, 2:10 AM

Remove redundant new line

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.

mehdi_amini accepted this revision.Jan 10 2022, 12:42 PM

LG, thanks!

rsuderman accepted this revision.Jan 11 2022, 10:15 AM
This revision is now accepted and ready to land.Jan 11 2022, 10:15 AM