The pass still drops unit dims of Linalg ops.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
I tried this change with IREE and did not see any failures. (I was not running various iree/tests/e2e tests.)
Comment Actions
Please don't remove these patterns. If these patterns are interfering with things please split them into two populate* methods
Comment Actions
These patterns prevent the opposite transforms from becoming canonicalization patterns.
Because of tensor::CollapseShapeOp::getCanonicalizationPatterns(patterns, context);.
/// Patterns that are used to canonicalize the use of unit-extent dims for
/// broadcasting.
void mlir::linalg::populateFoldUnitExtentDimsPatterns(
RewritePatternSet &patterns) {
auto *context = patterns.getContext();
patterns.add<FoldUnitDimLoops, AddInitOperandsToInput, ReplaceUnitExtents,
RankReducedExtractSliceOp,
RankReducedInsertSliceOp<tensor::InsertSliceOp>,
RankReducedInsertSliceOp<tensor::ParallelInsertSliceOp>>(
context);
linalg::FillOp::getCanonicalizationPatterns(patterns, context);
tensor::CollapseShapeOp::getCanonicalizationPatterns(patterns, context);
tensor::EmptyOp::getCanonicalizationPatterns(patterns, context);
tensor::ExpandShapeOp::getCanonicalizationPatterns(patterns, context);
memref::populateResolveRankedShapeTypeResultDimsPatterns(patterns);
memref::populateResolveShapedTypeResultDimsPatterns(patterns);
}I'd like to understand if and why these patterns are needed.
Comment Actions
Dropping unit dims with expand/collapse is very problematic for a quite a bunch of reasons, they even trigger cases where we cannot statically know how to lower to LLVM (@qcolombet has been digging here).
Short-term, I am not opposed to keeping those in a quarantined populate* for now if you have a compelling use case, but just be aware that they need to go and the expand/collapse verifier is likely going to be tightened to disallow the ambiguous cases, which will make these patterns fail in certain cases.
The right way to drop 1s is via insert/extract_slice that also work generally with tiling.
We're still gathering data but please share compelling use cases that we would need to take into account.
The fact that IREE does not seem affected by this is a strong data point that Matthias provides.
Comment Actions
I am not even sure you looked at what the pattern is trying to do, it is trying to coax the program to use rank-reducing extracts and inserts. This is used in IREE basically because the input program does NOT use rank-reduced extracts and insert slices, and these patterns were trying to get there. The reshapes introduced are for either the result of the slice where the unit dims need to be rematerialized for satisfying users (these are cleaned up later), or to get the source of the insert slice into the shape to make the tensor.insert_slice rank-reducing.
We're still gathering data but please share compelling use cases that we would need to take into account.
The fact that IREE does not seem affected by this is a strong data point that Matthias provides.
There might not be failures, but that doesnt mean they arent affected. We dont track all the metrics in CI that might affect this (CI infra is still being built up to track all of these). These were uncovered while looking through models and reading through the IR coming from the input. So these changes might not show failures but show up later when evaluating models. CI on IREE side needs to get better to be able to catch material changes on patches (thats WIP), but also using lit tests dont allow you to check whole model behavior.
Comment Actions
I am really confused. Adding these as canonicalization patterns is a red flag for me. We've been burnt by adding too many things into canonicalizers, but that same rubric is not being applied in this case?
Because of tensor::CollapseShapeOp::getCanonicalizationPatterns(patterns, context);.
/// Patterns that are used to canonicalize the use of unit-extent dims for /// broadcasting. void mlir::linalg::populateFoldUnitExtentDimsPatterns( RewritePatternSet &patterns) { auto *context = patterns.getContext(); patterns.add<FoldUnitDimLoops, AddInitOperandsToInput, ReplaceUnitExtents, RankReducedExtractSliceOp, RankReducedInsertSliceOp<tensor::InsertSliceOp>, RankReducedInsertSliceOp<tensor::ParallelInsertSliceOp>>( context); linalg::FillOp::getCanonicalizationPatterns(patterns, context); tensor::CollapseShapeOp::getCanonicalizationPatterns(patterns, context); tensor::EmptyOp::getCanonicalizationPatterns(patterns, context); tensor::ExpandShapeOp::getCanonicalizationPatterns(patterns, context); memref::populateResolveRankedShapeTypeResultDimsPatterns(patterns); memref::populateResolveShapedTypeResultDimsPatterns(patterns); }I'd like to understand if and why these patterns are needed.