Turn on MaterializeEncodingIntoNop pass to a later stage and for all backends
#17817
Conversation
lialan
force-pushed
the
lialan/nop
branch
4 times, most recently
from
6768f78
to
3bbf88f
Compare
lialan
requested review from
hanhanW,
antiagainst,
MaheshRavishankar,
kuhar,
qedawkins and
Groverkss
as code owners
hanhanW
added
benchmarks:cuda
There was a problem hiding this comment.
Interesting, I don't expect that there are changes in MaterializeHomogeneousEncodings.cpp because it could introduce new graph to other backends on default path. E.g., the padding and encodings ops are formed into a single dispatch if there are multi targets. We will delete the data-tiling passes from GlobalOpt (or move them to preprocessing phase) once we finish #17722. I'd suggest to not change this file for multi-device project stability. We should be able to develop data-tiling under some flags. In terms of test coverage, I think we can add few e2e matmul suite to other backends like
iree/tests/e2e/matmul/BUILD.bazel
Lines 214 to 224 in acc3558
I.e., we can create one suite for each backend and add below two additional compilation flags to the suites.
"--iree-opt-data-tiling",
"--iree-global-opt-enable-early-materialization=false",
a9a52ee
to
d43c1f5
Compare
|
@hanhanW Can you review again? I added specific tests to other backends, but hit problems with spirv with unsupported data types in legalizing pass. Took me a while but haven't figured a simple datatype choice to pass spriv. I think this is a spirv specific issue (that some data types are not being supported) and is not related to the scope of this patch. So I think we should just skip this test. |
There was a problem hiding this comment.
Nice! Can you also add tests for amdgpu? Unlike other targets, the amdgpu tests are only available in CMakeLists.txt. That's why you don't see any of them in the BUILD.bazel. I think we can add the test suite for both CDNA and RDNA3.
iree/tests/e2e/matmul/CMakeLists.txt
Line 2160 in d174e8b
| { | ||
| FunctionLikeNest newFuncPassManager(funcPassManager); | ||
| addEncodingToNopPasses(newFuncPassManager); | ||
| } |
There was a problem hiding this comment.
I think we don't need the change for VMVX side either. It should already be handled by CPUMaterializeEncoding pass.
iree/compiler/src/iree/compiler/Dialect/VMVX/Transforms/Passes.cpp
Lines 42 to 51 in d174e8b
Note: The VMVX pipeline is quite different from other backends, so you don't find the pass in this file.
I'm currently refactoring |
hanhanW
dismissed
their stale review
I want to take a look at spirv failure, dismiss my approval for now
compiler/src/iree/compiler/Codegen/Common/MaterializeEncodingIntoNop.cpp
Outdated
Show resolved
Hide resolved
|
@ScottTodd perhaps can you also help a bit here? https://github.com/iree-org/iree/actions/runs/9898353931/job/27345667940?pr=17817#step:15:144 It has reached binary size limit. |
Working on it: #17873 |
|
@hanhanW CI is now good, please review it again. |
lialan
force-pushed
the
lialan/nop
branch
2 times, most recently
from
e446f54
to
11be982
Compare
lialan
removed
benchmarks:android-cpu
Signed-off-by: Alan Li <[email protected]>
Abbreviated Benchmark Summary@ commit d196a9b084d5d5aaf4266bbec9e7c706ecac834e (vs. base be461bd0c17d9e607a316b8312bdc0f62298f581) Data-Tiling Comparison TableClick to show
No improved or regressed benchmarks 🏖️ No improved or regressed compilation metrics 🏖️ For more information: |
|
When merging pull requests, please keep the PR title and description in the merged commit. That should be the default behavior. https://iree.dev/developers/general/contributing/#merging-approved-changes This was merged as 2ed3f92, with a confusing commit title and no commit body: (btw, during code reviews please also prefer to push new commits instead of force pushing, as separate commits make it easier for reviewers to see what changed after review rounds) |
This is a prerequisite for disabling early materialization pass.
Issue: #17719