Page MenuHomePhabricator

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

Authored by arnab-oss on Fri, Nov 19, 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.Fri, Nov 19, 6:00 AM
arnab-oss requested review of this revision.Fri, Nov 19, 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.Sat, Nov 20, 4:43 AM
bondhugula requested changes to this revision.Sat, Nov 20, 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.Sat, Nov 20, 4:50 AM
bondhugula added inline comments.Sat, Nov 20, 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 marked 3 inline comments as done.

Addressed comments by @bondhugula.

bondhugula added inline comments.Mon, Nov 22, 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.Mon, Nov 22, 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.Mon, Nov 22, 3:48 AM
arnab-oss marked an inline comment as done.

Addressed comments by @nicolasvasilache and @bondhugula.

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

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

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

thanks for the fix!

bondhugula accepted this revision.Thu, Nov 25, 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.Thu, Nov 25, 9:39 AM
arnab-oss marked an inline comment as done.

Removed extra parenthesis as suggested by @bondhugula.

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

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

arnab-oss marked an inline comment as done.

Rebased on latest master.