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

[Renato Golin]

rengolin wrote:

> So I’d strongly prefer we keep the contract MemRef-only, and not have it “also happen to work” for tensors. If an op’s base is a tensor, having this interface still be “implementable” is confusing and makes it harder to reason about what the interface guarantees.

I second this. We had a few examples of this over the years and it always ends up in misunderstandings across different teams.

When implementing `pack` and `unpack`, we decided to restrict it to tensors because making it generic was too expensive and leaving the door open would create confusion. Since, it has been added to memrefs, but not by transition: there were a lot of discussion and PRs to reach there.

Unless the interface is meant to be used with _unknown_ downstream _"memref-like"_ types, in which case, it should not be in the `memref` dialect.

> in a stack or not does not change how I look at a PR (other than providing some context), the PR should be standalone and fully tested in itself.

This is not always true. When adding new targets, for example, we allow tests to be added later because it's not possible to test things early on, but as long as there's a stack and the tests exist, we can see what they'll be. 

While this is not at that scale, I can understand the argument of creating duplication for the sake of early tests, only to remove in a follow up commit. If the PR with tests is always rebased onto this one, and that PR is green at all times, than the likelihood of such case is diminished. I'd only ask to rebase that PR on this one before merge and only merge this one once that one is green.



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