[Mlir-commits] [mlir] [mlir] Refactor ConvertVectorToLLVMPass options (PR #128219)

[Andrzej Warzyński]

================
@@ -1410,10 +1410,32 @@ def ConvertVectorToLLVMPass : Pass<"convert-vector-to-llvm"> {
            "bool", /*default=*/"false",
            "Enables the use of X86Vector dialect while lowering the vector "
 	   "dialect.">,
-    Option<"vectorTransformsOptions", "vector-transform-options",
-           "vector::VectorTransformsOptions",
-           /*default=*/"vector::VectorTransformsOptions()",
-           "Options to lower some operations like contractions and transposes.">,
+    Option<"vectorContractLowering", "vector-contract-lowering",
+           "vector::VectorContractLowering",
+           /*default=*/"vector::VectorContractLowering::Dot",
+           VectorContractLoweringAttr.summary, [{::llvm::cl::values(
+           clEnumValN(::mlir::vector::VectorContractLowering::Dot, "dot", 
+            "Progressively lower to finer grained `vector.contract` and dot-products. (default)"),
+           clEnumValN(::mlir::vector::VectorContractLowering::Matmul, "matmul",
+            "Lower to `vector.matrix_multiply`, maps 1-1 to LLVM matrix intrinsics."),
+           clEnumValN(::mlir::vector::VectorContractLowering::OuterProduct, "outerproduct",
+            "Lower to `vector.outerproduct`."),
+           clEnumValN(::mlir::vector::VectorContractLowering::ParallelArith, "parallelarith",
+            "Lower contract with all reduction dimensions unrolled to 1 to a vector elementwise operations.")
+	        )}]>,
+    Option<"vectorTransposeLowering", "vector-transpose-lowering",
+           "vector::VectorTransposeLowering",
+           /*default=*/"vector::VectorTransposeLowering::EltWise",
+           VectorTransposeLoweringAttr.summary, [{::llvm::cl::values(
+           clEnumValN(::mlir::vector::VectorTransposeLowering::EltWise, "eltwise",
+            "Lower transpose into element-wise extract and inserts (default)"),
+           clEnumValN(::mlir::vector::VectorTransposeLowering::Flat, "flat",
+            "Lower 2-D transpose to `vector.flat_transpose`, maps 1-1 to LLVM matrix intrinsics"),
+           clEnumValN(::mlir::vector::VectorTransposeLowering::Shuffle1D, "shuffle1d",
+            "Lower 2-D transpose to `vector.shuffle` on 1-D vector."),
+           clEnumValN(::mlir::vector::VectorTransposeLowering::Shuffle16x16, "shuffle16x16",
+            "Lower 2-D transpose to `vector.shuffle` on 16x16 vector.")
+          )}]>,
----------------
banach-space wrote:

> The https://github.com/llvm/llvm-project/pull/123491 added these options to the pass initially so I'm assuming that this implies the need for this kind of control?

Well, the lack of upstream testing would suggest otherwise. :)  

Now, there's a good reason for that - these transformations are tested via Transform Dialect (TD). However, without explicit tests, it becomes very difficult to distinguish between **dead code** and actively used functionality. And that’s my biggest concern.  

That said, the test you added for **serializing the options** is a great idea to mitigate this - thanks!  

Still, there’s no clear indication (in-tree) that this functionality is actually needed.

---

> Indeed, there are a large number of test passes registered [here](https://github.com/llvm/llvm-project/blob/main/mlir/tools/mlir-opt/mlir-opt.cpp#L34) that would suggest so, unless I've misunderstood.

Yes, but there’s a comparable number of Transform Dialect [Ops](https://mlir.llvm.org/docs/Dialects/Transform/).  

Let’s be a bit more specific:  
- **27** test files in `mlir/test/Dialect/Tensor` → **7** use TD.  
- **158** test files in `mlir/test/Dialect/Linalg` → **79** use TD.  
- **71** test files in `mlir/test/Dialect/Vector` → **29** use TD.  

(I focused on the "Tensor compiler" specifically.)  

So, TD is **not the dominant driver** for test pipelines, but it's also **not just a side infrastructure**. I suspect it’s more commonly used where fine-grained control is required - for example, [here](https://github.com/llvm/llvm-project/blob/60cc3af0d93ecb8bfc9d6bebc6cbc395df3bb4b6/mlir/test/Dialect/Vector/vector-transpose-lowering.mlir#L129).

Now, note that _this PR_ adds:
```cpp
clEnumValN(::mlir::vector::VectorTransposeLowering::Shuffle1D, "shuffle1d",
            "Lower 2-D transpose to `vector.shuffle` on 1-D vector."),
```

So the key question is:
* **Do we duplicate the test to verify that the newly supported flag works as expected?**

> As a result, my hope is that the correctness of these specific patterns in this PR is covered by existing tests in the vector dialect.

Absolutely! Adding **new** `Vector` tests would be way beyond the scope of this PR 😅 

> Would it be acceptable to add the default enum values to tests already using this pass and also add more flag combinations to the dump pipeline test?

I'm not sure I follow ...

My thinking is more along the lines of **adding**  new `RUN` lines to e.g. https://github.com/llvm/llvm-project/blob/60cc3af0d93ecb8bfc9d6bebc6cbc395df3bb4b6/mlir/test/Dialect/Vector/vector-transpose-lowering.mlir. For example, we could add:
```mlir
mlir-opt -vector-transpose-lowering=shuffle1d %s | FileCheck --check-prefix=SHUFFLE_1D
```

while still keeping the existing:
```mlir
// RUN: mlir-opt %s --transform-interpreter --split-input-file | FileCheck %s
```

The downside? `check-prefix` explosion :(

----

@dcaballe, I am guessing that you will use this downstream? If yes, then lets merge this - we should aim to progress 
* https://discourse.llvm.org/t/mlir-specifying-structs-as-cl-options-for-passoptions/ 

ASAP. Also, I am not really providing any viable alternatives.

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