Page MenuHomePhabricator

[MLIR] Avoid creation of buggy affine maps when incorrect values of number of dimensions and number of symbols are provided.
ClosedPublic

Authored by arnab-oss on Fri, Nov 19, 5:50 AM.

Details

Summary

We check whether the maximum index of dimensional identifier present
in the result expressions is less than dimCount (number of dimensional
identifiers) argument passed in the AffineMap::get() and the maximum index
of symbolic identifier present in the result expressions is less than
symbolCount (number of symbolic identifiers) argument passed in AffineMap::get().

Diff Detail

Event Timeline

arnab-oss created this revision.Fri, Nov 19, 5:50 AM
arnab-oss requested review of this revision.Fri, Nov 19, 5:50 AM
rriddle added inline comments.Fri, Nov 19, 11:18 AM
mlir/lib/IR/MLIRContext.cpp
1027–1046

Instead of recursion, can you use a worklist?

bondhugula added inline comments.Sat, Nov 20, 4:43 AM
mlir/lib/IR/MLIRContext.cpp
1027–1046

The recursion here is no different than the recursion that happens when parsing affine expressions, walking, simplifying, or processing affine expressions. Using a worklist would be harder to read. Note that this is recursing over existing IR (left and right sub-trees of binary exprs) -- it's bounded and O(IR size). Such recursion is allowed, right?

bondhugula added inline comments.Sat, Nov 20, 4:57 AM
mlir/lib/IR/MLIRContext.cpp
39

I think you won't need this include.

nicolasvasilache requested changes to this revision.Sat, Nov 20, 8:17 AM
nicolasvasilache added inline comments.
mlir/lib/IR/MLIRContext.cpp
1020

You can write this check in 3 lines by calling AffineMap::inferFromExprList and checking against the returned AffineMap.

This revision now requires changes to proceed.Sat, Nov 20, 8:17 AM

Addressed comments.

Removed unnecessary #include<algorithm>.

arnab-oss marked an inline comment as done.Sun, Nov 21, 11:07 PM

ping @rriddle and @nicolasvasilache can you please take a look at the replies of your respective comments.

mlir/lib/IR/MLIRContext.cpp
1020

AffineMap::inferFromExprList calls AffineMap::get() internally, which causes a layering issue as we are indirectly calling AffineMap::get() from inside AffineMap::get(). But before calling AffineMap::get() using a similar traversal. So, first creating a correct map separately, and then checking whether numDims and numSymbols follow the constraints will do an extra map allocation, as it will call the getImpl() for creating the correct map. Our aim is to not create any map at all if the arguments are wrong.
CC - @bondhugula

mlir/lib/IR/MLIRContext.cpp
1020

Sure, but I imagine you can think of ways of layering this differently?

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.

arnab-oss marked 4 inline comments as done.Thu, Nov 25, 9:26 AM

@nicolasvasilache, @bondhugula and @rriddle, I have addressed all your comments. Please take a look.

This revision is now accepted and ready to land.Thu, Nov 25, 12:20 PM

Rebased on latest master.

bondhugula accepted this revision.Fri, Nov 26, 10:48 AM
bondhugula added inline comments.
mlir/include/mlir/IR/AffineMap.h
550

Missing doc comment here - it wasn't there earlier, but we should add one now that this is publicly exposed.

arnab-oss marked an inline comment as done.

Added doc comments for the getMaxDimAndSymbol().

bondhugula accepted this revision.Fri, Nov 26, 11:05 AM
bondhugula added inline comments.
mlir/include/mlir/IR/AffineMap.h
549

Nit: position -> positions

in the `exprsLists` -> in `exprsLists`
This revision was landed with ongoing or failed builds.Fri, Nov 26, 11:09 AM
This revision was automatically updated to reflect the committed changes.