This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Rank reducing subview conversion to LLVM

Authored by limo1996 on Oct 6 2020, 2:57 AM.



This commit adjusts SubViewOp lowering to take rank reduction into account.

Depends On D88879

Diff Detail

Event Timeline

limo1996 created this revision.Oct 6 2020, 2:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
limo1996 requested review of this revision.Oct 6 2020, 2:57 AM
nicolasvasilache requested changes to this revision.Oct 6 2020, 3:14 AM

Let's please extract this into a helper function.
If I am not mistaken this could also be reused in Ops.cpp for isRankReduced?


This should not be exposed to Ops.h and ideally deprecated.
The problem is at this time the result types of map_range and ArrayAttr::getAsValueRange are unfathomble.

This is a rare case where I'd say it's ok to copy the function where you need it (and keep it static) for now.

This revision now requires changes to proceed.Oct 6 2020, 3:14 AM
limo1996 updated this revision to Diff 296908.Oct 8 2020, 3:02 AM
limo1996 marked 2 inline comments as done.

Comments of Nicolas incorporated

nicolasvasilache requested changes to this revision.Oct 8 2020, 3:34 AM
nicolasvasilache added inline comments.

The name of the function and the doc comment are quite misleading.
From this, I wouldn't know how to find this functionality in 1 month once this gets out of my short-term memory...

I'd suggest :

Given an `originalShape` and a `reducedShape` assumed to be a subset of `originalShape` with some `1` entries erased, return the vector of booleans that specifies which of the entries of `originalShape` are keep to obtain `reducedShape`.
The returned mask can be applied as a projection to `originalShape` to obtain the `reducedShape`.
This mask is useful to track which dimensions must be kept when e.g. compute MemRef strides under rank-reducing operations.
Return None if reducedShape cannot be obtained by dropping only `1` entries in `originalShape`.

llvm::Optional<SmallVector<bool, 4>> computeRankReductionMask(ArrayRef<int64_t> originalShape, ArrayRef<int64_t> reducedShape);

Feel free to further improve amd/or find a better name if one comes to mind.


reducedIdx is misleading, it should be called numReducedDims.

Also, it seems that you only use this in the verification.
So the other use of the API is less intuitive.

If you implement this with returning an llvm::Optional<SmallVector<bool, 4>>, you can test on whether a value is actually returned.


reads somewhat hacky with the hardcoded -2.

Let's please rewrite as:

keepMask[originalIdx] = (reducedIdx < reducedRank) && 
  (originalShape[originalIdx] == reducedShape[reducedIdx]);
if (keepMask[originalIdx])
if (reducedRank != std::count_if(keepMask.begin(), keepMask.end(), [](bool b){ return b; }))
  return ...;
if (!keepMask.hasValue())

Seems like, in the limit, this could be defined on empty shapes too.
Returning llvm::None is a clear error failure that reducedShape cannot be obtained from originalShape by dropping only 1's.

This revision now requires changes to proceed.Oct 8 2020, 3:34 AM
limo1996 updated this revision to Diff 296948.Oct 8 2020, 6:29 AM
limo1996 marked 5 inline comments as done.

Next wave of comments addressed

This revision is now accepted and ready to land.Oct 8 2020, 6:38 AM
This revision was automatically updated to reflect the committed changes.