Page MenuHomePhabricator

[mlir][MemRef] Compute unused dimensions of a rank-reducing subviews using strides as well.
ClosedPublic

Authored by mravishankar on Sep 7 2021, 11:13 PM.

Details

Summary

For memref.subview operations, when there are more than one
unit-dimensions, the strides need to be used to figure out which of
the unit-dims are actually dropped.

Diff Detail

Event Timeline

mravishankar created this revision.Sep 7 2021, 11:13 PM
mravishankar requested review of this revision.Sep 7 2021, 11:13 PM

Sorry for delay.

How does this change the situation for subviews with dynamic strides? Dynamic strides will all have the same static value so it looks like the code will just assume all dimensions are dropped because of that.

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
693

Nit: please document and drop trivial braces below.

725

Typo: dimension

730

Did you mean size rather than rank?

732

Nit: spurious empty line

744
756–758

If this can never happen, it should be an assertion. I see you are asserting below, but maybe let's better to assert here and put your comment as assertion message + have different messages for different that currently return None.

mlir/test/Dialect/MemRef/invalid.mlir
365

Nit: please add a newline

sorry I should have commented here .. I will pick this up and rewrite some of the basics, this should make the whole thing much smaller code-wise.

Sorry for delay.

How does this change the situation for subviews with dynamic strides? Dynamic strides will all have the same static value so it looks like the code will just assume all dimensions are dropped because of that.

Ill explain by example. Lets say you have two unit dimensions with dynamic strides, s0 and s1, if either of one is dropped, then the result will have only one dynamic stride s0 irrespective of which one is dropped. So this is handled (I also have a test for this I think).

sorry I should have commented here .. I will pick this up and rewrite some of the basics, this should make the whole thing much smaller code-wise.

Thats fine, as long as the tests here pass. (In any case, I think the logic here is actually quite simple, it is the simplest of all candidates I could come up with (dont think number of lines of codes correspond to simplicity :) ). I am not tied to the solution though, so please let me know when you have this approach working.

Let me unblock this for now and I'll come back to it when I have cycles.

This revision is now accepted and ready to land.Sep 16 2021, 11:45 AM
mravishankar marked 6 inline comments as done.

Rebase and address comments.

@ftynse addressed most of your comments. I am running presubmits, and will submit after they pass. Ill address any other comments you have post-submit.

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
756–758

Well, this is used in the verifier as well, so it needs to return on error and not assert. I could have different messages, but that would just be little verbose. Finally its about type mismatch. The caller reports it as such.

Change from using DenseMap<int64_t> to std::map<int64_t> since the dynamic stride value is same as tombstone value for DenseMap<int64_t>.

ftynse accepted this revision.Sep 20 2021, 11:02 AM