[Mlir-commits] [mlir] [mlir] Implement indexed access op interfaces for memref, vector, gpu, nvgpu (PR #177014)

[Krzysztof Drewniak]

krzysz00 wrote:

Ok, so
1. Yeah, a PR structure where I land the patterns from fold-memref-alias-ops and implementations on `memref` so there's testable code there makes sense if that would be easier to review. There's also a patch I haven't written yet where I port the other big switch/case over memref accessing operations (the one that extracts indexing into subviews) to this interface, but that'll be future work.
2. @banach-space , re vector, I'm trying to understand if you have fundamental objections to implementing `memref::IndexedAccessOpInterface` on operations that might also have tensor arguments being indexed at some point in their lifecycle or if this is more a documentation concern (or a case of something like "we could use some asserts"). If it's a documentation/hardening concern, I'm trying to work out what it is you want to see improved - I'm afraid I just don't see the footgun you're worried about.
3. Re the shape_cast point, I suppose I could break that up, but part of the intent of this interface is to allow for this sort of complex handling (ex. the fact that if I'm loading a `vector<1x8xf32>` from a `memref<MxNxf32>` which is actually `expand_shape`d from a `memref<M * Nxf32>` should allow the expand_shape to be folded in in a way that isn't allowed with a `vector<2x8xf32>` - but the logic needed to enable that should live near `vector.load`, not over in Dialects/MemRef.
And to quote the method semantics (\*\* added here)
```
    InterfaceMethod<
      /*desc=*/[{
        Return the shape of the portion of the memref that is being accessed by
        this operation, if known, **ignoring leading unit dimensions**.

        Reindexing transformations may not modify the *strides* of the trailing
        N dimensions, where N is the size returned value, and should ensure that
        at least N indexing dimensions remain after the transformation.
      }],
      /*retType=*/"::llvm::SmallVector<int64_t>",
```
(I'll fix the typos in there Soon)

https://github.com/llvm/llvm-project/pull/177014