[Mlir-commits] [mlir] [mlir][linalg] Restrict linalg.pack to not have artificial padding. (PR #149624)

[Han-Chung Wang]

================
@@ -150,9 +150,10 @@ def Linalg_PackOp : Linalg_RelayoutOp<"pack", [
 
     `padding_value` specifies a padding value at the boundary on non-perfectly
     divisible dimensions. Padding is optional:
-    - If absent, it is UB if the tile does not perfectly divide the dimension.
+    - If absent, it assumes the tile perfectly divides the dimension.
     - If present, it will pad along high dimensions (high-padding) to make the
-      tile complete.
+      tile complete. Note that it is not allowed to have artificial padding that
+      is not strictly required by linalg.pack.
----------------
hanhanW wrote:

> It could make sense to disallow static number of packed tiles when packed dimension is dynamic.

I'd be careful to have such check because shape inference is possible. E.g.,

https://github.com/llvm/llvm-project/blob/bbe912f1e74252f1f57b5d6c6d97a418947f4aa0/mlir/test/Dialect/Linalg/canonicalize.mlir#L1793-L1815

To be more specific, you can have `linalg.generic -> linalg.pack -> linalg.generic` program, where the `linalg.pack` is fully dynamic and `linalg.generic` ops are partial dynamic. For some cases, you can fully infer the static shapes for `linalg.generic`, so it becomes `linalg.generic -> tensor.cast -> linalg.pack -> tensor.cast -> linalg.generic`. Here, you have the chance to fold `tensor.cast` ops into `linalg.pack` ops; it has fully static shape now.

If we have such constraint, illegal IR can be introduced during the transformation. Furthermore, you can have such IR, infer the static shape and fold cast ops away.


> For static it's easy to verify.
> For dynamically shaped source, linalg::lowerPack doesn't respect that i.e., extra padding will be applied.
>
> Not sure if it's really a problem but FYI.

Thanks for the information! I think maybe I can add some doc and say that it is UB if extra padding is requested. WDYT?

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