[Mlir-commits] [mlir] [memref] Simplify loads from reinterpret_cast of 1D contiguous memrefs (PR #188459)

[Andrzej Warzyński]

================
@@ -196,6 +198,234 @@ struct CopyToScalarLoadAndStore : public OpRewritePattern<memref::CopyOp> {
   }
 };
 
+/// Describes the unique non-unit dimension of a MemRef shape.
+///
+/// This helper is only used for shapes that have at most one non-unit
+/// dimension. `exists` is false for all-ones shapes. Otherwise, `isOnLeft`
+/// indicates whether the non-unit dimension is on the left boundary.
+///
+/// If `exists` is true and `isOnLeft` is false, the non-unit dimension is on
+/// the right boundary. Rank-1 non-unit MemRefs are treated as matching both
+/// boundaries and callers that care about the right boundary must account for
+/// that from the MemRef type.
+struct SingleNonUnitDimInfo {
+  bool exists = false;
+  bool isOnLeft = false;
+};
----------------
banach-space wrote:

I was re-reading this and find `SingleNonUnitDimInfo` a bit odd and counter-intuitive. It's actually quite hard to come up with any suggestion, but let me try:


```suggestion
/// Describes a 1D MemRef shape with either leading or trailing unit dims
///
/// Captures info about MemRefs that are effectively 1D (the leading or trailing dims are all 1).
/// The only allowable non-unit dim is either the leading of the trailing dim. Examples:
///
/// memref<1x1x4xf32>, memref(4x1x1xf32>, memref<1x1x1xf32>
///
struct 1DMemRefShapeInfo {
  // Are all dims == 1? `false` means that there is exactly one dim that's != 1.
  bool allOnes = true;
  // If there is a non-unit boundary dim, is it the leading or the trailing dim?
  bool isLeadingDimNonUnit = false;
};
```

1. To me, using `trailing`/`leading` (vs "left" and "right") is more native to compilers.
2. `exists` (your original field) wasn't, IMHO, descriptive enough.
3. `SingleNonUnitDimInfo` vs `1DMemRefShapeInfo` - I think that using `1D` in the name helps to emphasise that this is for some degenerate cases.

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