[Mlir-commits] [mlir] [MLIR] Determine contiguousness of memrefs with dynamic dimensions (PR #142421)

[Andrzej Warzyński]

banach-space wrote:

The latest updates don’t address all of my concerns - I’ve “unresolved” the corresponding threads to keep the discussion visible.

Broadly, I agree with Alex’s point that we should test functionality like this via transformations rather than unit tests. From [the LLVM Testing Guide](https://llvm.org/docs/TestingGuide.html#unit-tests):

>  In general unit tests are reserved for targeting the support library and other generic data structure, we prefer relying on regression tests for testing transformations and analysis on the IR.

`MemRefType::getNumContiguousTrailingDims` isn’t a support-library hook. That said, I do see value in unit-testing this specific case (see my [comment](https://github.com/llvm/llvm-project/pull/142421#discussion_r2152187179)), so I’m OK with the current direction.

That said, we should be mindful that this takes a somewhat _non-canonical_ approach to testing in MLIR. With that in mind, I’d suggest trying to follow existing conventions as closely as possible:
* _We don’t typically test every variation of every API_ - testing `getNumContiguousTrailingDims` is sufficient here. No need to duplicate coverage via `areTrailingDimsContiguous`.
* _Documentation in tests is encouraged_. For example, see [this comment](https://github.com/banach-space/llvm-project/blob/37ebb1d54736d3c60b90b60018e7a2bbd3287573/mlir/unittests/IR/AttrTypeReplacerTest.cpp#L17-L19). In many cases, clear and descriptive names are enough. Either approach works, but names like m6 don’t convey intent. Even a short (block) comment would help.
* _Please reduce the number of test cases._ I’m not aware of any precedent for testing such a small utility function this extensively.

We don’t have formal documentation for MLIR unit test conventions, so this is based on experience and precedent.

I'm happy for other reviewers to suggest a different direction for this PR. From my side, the priority is to unblock this while:
* Taking all feedback into account.
* Adhering to existing conventions as much as we reasonably can.

Thank you all!

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