[Mlir-commits] [mlir] [mlir] Rewrites for I2 to I8 signed and unsigned extension (PR #121298)

[Andrzej Warzyński]

================
@@ -1084,10 +1084,14 @@ static LogicalResult alignedConversionPrecondition(PatternRewriter &rewriter,
   unsigned srcElemBitwidth = srcType.getElementTypeBitWidth();
   unsigned dstElemBitwidth = dstType.getElementTypeBitWidth();
 
-  // Only {s}i4 -> (size_of({{s}i/f}) >= 8) are supported for now.
-  if (srcElemBitwidth != 4 || dstElemBitwidth < 8 ||
-      (dstElemBitwidth % srcElemBitwidth) != 0)
-    return rewriter.notifyMatchFailure(op, "Not a supported aligned case");
+  if (dstElemBitwidth < 8)
+    return rewriter.notifyMatchFailure(
+        op, "the bitwidth of dstType must be greater than or equal to 8");
+  if (dstElemBitwidth % srcElemBitwidth != 0)
+    return rewriter.notifyMatchFailure(op, "unaligned cases are not supported");
+  if (srcElemBitwidth != 2 && srcElemBitwidth != 4)
+    return rewriter.notifyMatchFailure(
+        op, "only src bitwidth of 2 or 4 is supported at this moment");
----------------
banach-space wrote:

There's a few issues with this method (most are unrelated to this PR):
* We mix `srcType` and `dstType` - it's not clear which is which. See [example 1](https://github.com/llvm/llvm-project/blob/f99b1907570aa1ac3c8c0ff886563766bbdbc1c8/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L1434-L1436) vs [example 2](https://github.com/llvm/llvm-project/blob/f99b1907570aa1ac3c8c0ff886563766bbdbc1c8/mlir/lib/Dialect/Vector/Transforms/VectorEmulateNarrowType.cpp#L1488-L1490). From the POV of this hook, `srcType` and `dstType` are just a bit engimatic.
* This error is inaccurate: `return rewriter.notifyMatchFailure(op, "unaligned cases are not supported");` - note that every condition verified in this method contributes to "alignment verification". You probably meant: `dest element bitwdith is not a multiple of the source sub-byte`.
* The condition at the end, `(srcType.getShape().back() % 2)`, should be updated to `(srcType.getShape().back() % 4)` for `i2`, right?

Let's refine this hook a bit so that it's a bit clearer what exactly is being tested:
* https://github.com/llvm/llvm-project/pull/122136

I'm suggesting a separate PR as this one has already grown a bit 😅 

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