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

[Momchil Velikov]

================

----------------
momchil-velikov wrote:

> A few high-level comments on test organization and naming:
> 
> * `maxContigDim` tests `getNumContiguousTrailingDims`, `contigTrailingDim` tests `areTrailingDimsContiguous`, and `identityMaps` tests both - is there an intended pattern here?

`maxContigDim` is a remnant from when the member function was called `getMaxContiguousTrailingDims`.
I'll rename it.

'contigTrailingDims` tests `areTrailingDimsContiguous`, so it's named after the thing it tests.

`identityMaps` tests the fastpath when the memref has identify maps 
```
...
int64_t MemRefType::getNumContiguousTrailingDims() {
  const int64_t n = getRank();

  // memrefs with identity layout are entirely contiguous.
  if (getLayout().isIdentity())
    return n;
...
```

So, yeah, there's a pattern of naming the tests after the thing they test.

> * The actual "complex" logic seems to reside in `getNumContiguousTrailingDims`. Would it make sense to focus the tests more narrowly on that hook?

I find the tests focused enough.

> * Could the test names (`maxContigDim`,` contigTrailingDim`, `identityMaps`) be made more descriptive? Alternatively, adding comments explaining what each test is validating would help - it’s not immediately clear to me.

What each test is validating is abundantly clear from the `EXPECT_` lines.

> * I’d suggest renaming `identityMaps` to something like `noStrides` or `defaultStrides`, since the other tests explicitly include strides. The natural split seems to be "with strides" vs "without strides".

As explained above, it tests exactly the case for having identity maps, hence it's appropriately named.


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