This is an archive of the discontinued LLVM Phabricator instance.

[mlir][NFC] Extend 'getPermutedPosition' to support projected permutations
ClosedPublic

Authored by dcaballe on Nov 29 2022, 1:15 PM.

Details

Summary

Small change to support projected permutations in the
'getPermutedPosition' utility.

Diff Detail

Event Timeline

dcaballe created this revision.Nov 29 2022, 1:15 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
dcaballe requested review of this revision.Nov 29 2022, 1:15 PM
nicolasvasilache requested changes to this revision.Nov 29 2022, 1:21 PM

I am sorry but this API does not make sense to me, I think it was mistakenly added, IMO

Optional<int64_t> AffineMap::getResultPosition(AffineExpr)

makes much more sense and does not require Permutation, ProjectedPermutation, PermutationWithMaybeSomeBroadcast or other characterizations; if works with any AffineMap.

The client is responsible for assertions (it already is in this case).

Use unsigned if you really must (for maintaining consistency with neighbouring code) but we know this is not a good use case of unsigned by now and should be globally cleaned up in the future.

This revision now requires changes to proceed.Nov 29 2022, 1:21 PM
dcaballe updated this revision to Diff 478760.Nov 29 2022, 5:03 PM

I'm sorry, Nicolas, I'm not sure I follow what the AffineExpr input is.
I changed the name to getResultPos and removed the assert so that it works for any case.
Please, let me know if that is not what you wanted!

I'm sorry, Nicolas, I'm not sure I follow what the AffineExpr input is.
I changed the name to getResultPos and removed the assert so that it works for any case.
Please, let me know if that is not what you wanted!

The AffineExpr is what the client passes, it can be an AffineDimExpr(pos) or not.

Client-side:

assert(order.isPermutation());
auto maybePos = order.getFirstResultPosition(AffineDimExpr::get(d, ctx));
assert(maybePos.has_value());
return *maybePos;

AffineMap-side:

Optional<unsigned> AffineMap::getFirstResultPosition(AffineExpr desiredExpr) const {
  for (unsigned i = 0, numResults = getNumResults(); i < numResults; i++) {
    if (getResult(i) == desiredExpr)
      return i;
  }
  return llvm::None;
}

Actually, now that I write it, it looks a lot like a C++ find, so findResult ?

I think that is computing something different. The input in the original utility is an input dimension position and we want to figure out what the position of that dimension is in the result (assuming a projected permutation). We don't have the result expression.
For example, for (d0, d1, d2, d3) -> (d1, d0), the input to the utility would be 0, 1, 2 or 3, and the utility would return 1, 0, None, None, respectively, for those inputs. It's kind of applying the map to an input dimension position. Does it make sense?

I think that is computing something different. The input in the original utility is an input dimension position and we want to figure out what the position of that dimension is in the result (assuming a projected permutation). We don't have the result expression.
For example, for (d0, d1, d2, d3) -> (d1, d0), the input to the utility would be 0, 1, 2 or 3, and the utility would return 1, 0, None, None, respectively, for those inputs. It's kind of applying the map to an input dimension position. Does it make sense?

Yes absolutely, and taking your example, what I am suggesting is that the input to the utility would be AffineDimExpr::get(0, ctx), AffineDimExpr::get(1, ctx),AffineDimExpr::get(2, ctx),AffineDimExpr::get(3, ctx) and the utility would return 1, 0, None, None, respectively, for those inputs.
This makes the API take a generic AffineExpr input and avoid passing magic integers that get a special meaning.
This is more general and less surprising to use.

dcaballe updated this revision to Diff 479050.Nov 30 2022, 12:37 PM

Addressed feedback. Thanks!

nicolasvasilache accepted this revision.Dec 1 2022, 10:22 AM
nicolasvasilache added inline comments.
mlir/include/mlir/IR/AffineMap.h
169–172

Please update the comment.

Extracts the first result position where `input` resides.
Return llvm::None if `input` cannot be found.
This revision is now accepted and ready to land.Dec 1 2022, 10:22 AM
This revision was automatically updated to reflect the committed changes.