Page MenuHomePhabricator

[MLIR] Prevent creation of buggy affine map after linearizing collapsed dimensions of source map
ClosedPublic

Authored by arnab-oss on Nov 19 2021, 6:00 AM.

Details

Summary

Initially we were passing wrong numSymbols argument while calling
AffineMap::get() for creaating affine map with linearized result
expressions. The main problems was the number of symbols of the newly
to be created map may be different from that of the source map, as
new symbolic identifiers may be introduced while creating strided layout
linearized expressions.

Diff Detail

Event Timeline

arnab-oss created this revision.Nov 19 2021, 6:00 AM
arnab-oss requested review of this revision.Nov 19 2021, 6:00 AM
bondhugula retitled this revision from [MLIR] Prevent creation of buggy affine map after linearizing collapsed dimensions of source map. to [MLIR] Prevent creation of buggy affine map after linearizing collapsed dimensions of source map.Nov 20 2021, 4:43 AM
bondhugula requested changes to this revision.Nov 20 2021, 4:50 AM
bondhugula added inline comments.
mlir/lib/Dialect/Linalg/Transforms/ElementwiseOpFusion.cpp
390–410

You can completely get rid of hasEncounteredSymbol if you just actually compute numSymbols. You can drop maxSymbolPosition as well.

numSymbols = std::max(numSymbols, symbolExpr.getPosition() + 1);

A single variable will do and it's more natural and compact.

This revision now requires changes to proceed.Nov 20 2021, 4:50 AM
bondhugula added inline comments.Nov 20 2021, 4:52 AM
mlir/lib/Dialect/Linalg/Transforms/ElementwiseOpFusion.cpp
390–410

Just initialize numSymbols = 0 on the top and it'll take care of everything. No additional conditional needed as well.

411

numSymbol -> numSymbols

arnab-oss updated this revision to Diff 388702.Nov 20 2021, 7:24 AM
arnab-oss marked 3 inline comments as done.

Addressed comments by @bondhugula.

bondhugula added inline comments.Nov 22 2021, 3:40 AM
mlir/lib/Dialect/Linalg/Transforms/ElementwiseOpFusion.cpp
389

Nit: sourceMap in backticks.

407

I assume the existing test cases already exercise this. Update a test case to test this?

nicolasvasilache requested changes to this revision.Nov 22 2021, 3:48 AM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/Transforms/ElementwiseOpFusion.cpp
390

This revision is also duplicating existing functionality, and implementing it with recursion, just like in D114238.

Either: return AffineMap::inferFromtExprList(...);

Absolute worst case, you can expose:

template <typename AffineExprContainer>
static void getMaxDimAndSymbol(ArrayRef<AffineExprContainer> exprsList,
                               int64_t &maxDim, int64_t &maxSym) {

as a util and reuse it rather than reimplement a 5 line iterative algorithm with a longer recursive one.

This revision now requires changes to proceed.Nov 22 2021, 3:48 AM
arnab-oss updated this revision to Diff 389745.Nov 25 2021, 5:35 AM
arnab-oss marked an inline comment as done.

Addressed comments by @nicolasvasilache and @bondhugula.

arnab-oss marked an inline comment as done.Nov 25 2021, 8:43 AM

ping @nicolasvasilache, I have addressed your comments. You can take a look.

nicolasvasilache accepted this revision.Nov 25 2021, 9:00 AM

thanks for the fix!

bondhugula accepted this revision.Nov 25 2021, 9:39 AM
bondhugula added inline comments.
mlir/lib/Dialect/Linalg/Transforms/ElementwiseOpFusion.cpp
388

Do you need the extra parentheses?

This revision is now accepted and ready to land.Nov 25 2021, 9:39 AM
arnab-oss updated this revision to Diff 389828.Nov 25 2021, 9:52 AM
arnab-oss marked an inline comment as done.

Removed extra parenthesis as suggested by @bondhugula.

bondhugula accepted this revision.Nov 25 2021, 11:20 AM

This revision isn't properly based -- please rebase on the current master tip and update.

arnab-oss updated this revision to Diff 389882.Nov 25 2021, 1:45 PM
arnab-oss marked an inline comment as done.

Rebased on latest master.