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 Nov 19 2021, 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.Nov 19 2021, 5:50 AM
arnab-oss requested review of this revision.Nov 19 2021, 5:50 AM
rriddle added inline comments.Nov 19 2021, 11:18 AM
mlir/lib/IR/MLIRContext.cpp
1027–1046

Instead of recursion, can you use a worklist?

bondhugula added inline comments.Nov 20 2021, 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.Nov 20 2021, 4:57 AM
mlir/lib/IR/MLIRContext.cpp
39

I think you won't need this include.

nicolasvasilache requested changes to this revision.Nov 20 2021, 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.Nov 20 2021, 8:17 AM

Addressed comments.

Removed unnecessary #include<algorithm>.

arnab-oss marked an inline comment as done.Nov 21 2021, 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.Nov 25 2021, 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.Nov 25 2021, 12:20 PM
arnab-oss updated this revision to Diff 389883.Nov 25 2021, 1:54 PM

Rebased on latest master.

bondhugula accepted this revision.Nov 26 2021, 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.Nov 26 2021, 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.Nov 26 2021, 11:09 AM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Dec 3 2021, 1:25 PM
mlir/lib/IR/MLIRContext.cpp
1020

"nodiscard" isn't universally supported and it is a c++17 extension, so someone removed it after you committed this.

But more importantly: this is not desirable! What it means is to force the compiler to emit this function in the binary even if unused, there is a rarely a reason to do that, it'll just bloat the binary size.

I assume that the problem is the warning about this function being unused, in this case you can:

  1. Surround the function between #ifndef NDEBUG / #endif
  2. Use the LLVM_ATTRIBUTE_UNUSED decorator in front the function.