[Mlir-commits] [mlir] [mlir][vector] Teach `TransferOptimization` to forward masked stores (PR #87794)

Andrzej WarzyƄski llvmlistbot at llvm.org
Wed May 15 13:18:02 PDT 2024


================
@@ -170,12 +170,43 @@ AffineMap mlir::vector::getTransferMinorIdentityMap(ShapedType shapedType,
       shapedType.getContext());
 }
 
+/// Returns true if the value written by `defWrite` could be the same as the
+/// value read by `read`. Note: True is 'could be' not 'definitely' (as this
+/// simply looks at the masks and the value written). For a definite answer use
+/// `checkSameValueRAW()` -- which calls this function.
----------------
banach-space wrote:

Still, a `bool` method will always check a very specific condition which either holds or it does not. "could" in a function name suggests that this method is unable to return a clear answer. 

I think that the problem comes from the fact that the name is tied to where it's used (`checkSameValueRAW`). 

> This is a private/static method for implementing part of checkSameValueRAW(). The comment is to make clear that is function does not check enough to give a useful answer alone (so you likely don't want to call it -- you probably want checkSameValueRAW().

If this method "lives" outside of `checkSameValueRAW` then it should be able to document itself (without requiring one to scan `checkSameValueRAW`). Could you use a less ambiguous name? Here are some options:
* `haveIdenticalMasksAndMatchingPadAndSplatValues`
* `areMasksAndPadAndSplatValsConsistent`

I think that that would captures what's being checked? Alternatively, make this a lambda internal to ``checkSameValueRAW` (sounds like it's not meant to be used outside of `checkSameValueRAW` anyway).

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


More information about the Mlir-commits mailing list