[Mlir-commits] [mlir] [MLIR] Fix VectorEmulateNarrowType constant op mask bug (PR #116064)

[Andrzej Warzyński]

banach-space wrote:

> I think what really helps is to use more meaningful names in the test, it is quite self-explanatory!

Thank you for this comment and for updating the tests 🙏🏻. This makes reviewing and maintenance so much easier - our future selves will be very grateful! 😊

I’ve left a few small suggestions, but otherwise, this looks good to me (LGTM).

--- 
Given the growing complexity of this logic, I have a couple of high-level observations:

1. Should _"vector-emulate-narrow-type.mlir"_ be renamed to _"vector-emulate-narrow-type-aligned.mlir"_? Should the cases in _"vector-emulate-narrow-type-unaligned.mlir"_ essentially be variations of the cases in _"vector-emulate-narrow-type-aligned.mlir"_?

2. All the existing cases in "vector-emulate-narrow-type-unaligned.mlir" test `i2`, while the new cases test `i4`. In a PR like this, where the key "novelty" is support for `arith.constant` (i.e. a new Op), I’d normally expect the data types to remain consistent with the other tests. This way, it’s easier to isolate and understand the differences introduced by the new changes.

I’m not suggesting any additional large changes or refactors in this PR - @lialan has already done a great job! But I think the patterns/trends highlighted above emerge naturally and could serve as a helpful guiding principle for future PRs. If others agree, of course. 😊

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