[Mlir-commits] [mlir] Make createReadOrMaskedRead and isValidMaskedInputVector vector utilities (PR #89119)

[Andrzej Warzyński]

================
@@ -180,6 +180,23 @@ struct MaskableOpRewritePattern : OpRewritePattern<SourceOp> {
 /// are not linearizable.
 bool isLinearizableVector(VectorType type);
 
+/// Create a TransferReadOp from `source` with static shape `readShape`. If the
+/// vector type for the read is not the same as the type of `source`, then a
+/// mask is created on the read.
+/// enableMasking if false, the inBoundsVal values are set properly, based on
+///     the rank dimensions of the source and destination tensors.
----------------
banach-space wrote:

Please update this comment.

1. Fix formatting
2. "then a mask is created on the read." - the mask is never created if `enableMasking` is false. 
3. Missing note on `indices` being hard-coded as `0`.

Point 2. is rather counter-intuitive to me. @pashu123, you introduced `doMasking` in #88249. Looking at the implementation and you PR, it feels like you meant something like `bool useInBoundsInsteadOfMasking` rather than `doMasking`/`enableMasking`? As in:

BEFORE #88249:
```cpp
if (llvm::equal(readShape, sourceShape))
  // use masking
```

AFTER #88249:
```
if (llvm::equal(readShape, sourceShape) && !useInBoundsInsteadOfMasking)
  // use masking
```

Put differently, the extra logic added to `createReadOrMaskedRead ` in #88249 reads to me as:
*  "In some cases I want to disable masking and use in_bounds attr instead", rather than
* "I want a dedicated toggle to enable/disable masking".

I'm asking to better understand the current logic 😅 

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