[mlir] Refactor ConvertVectorToLLVMPass options #128219
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |
| #define MLIR_CONVERSION_PASSES | ||
|
|
||
| include "mlir/Pass/PassBase.td" | ||
| include "mlir/Dialect/Vector/Transforms/VectorTransformsBase.td" | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // ToLLVM | ||
|
|
@@ -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<"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.") | ||
| )}]>, | ||
abulavin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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.") | ||
| )}]>, | ||
|
There was a problem hiding this comment. IMHO, this is becoming too verbose and too fine-grained - do we need this? In general, there are three different ways to drive this pass (and the relevant transformations):
From my perspective, My bigger concern is that we are not testing these flags. If they duplicate existing Transform Dialect functionality, we should ensure that relevant RUN lines are duplicated, rather than adding tests only to check the flags themselves. This is a broader issue in MLIR and somewhat tangential to this PR (which is attempting to fix the issue reported here: MLIR: Specifying Structs as cl::Options for PassOptions). Let’s wait for others to chime in. More generally, I hope we can establish clearer guidelines on the relationship between different ways of "driving" transformations and preferred testing approaches. There was a problem hiding this comment. It would be news to me that transform dialect is the preferred way of testing something in MLIR: so far it's been an infrastructure on the side, not the primary way to access functionality (which remains passes and pass pipelines). There was a problem hiding this comment. The previous PR added these options to the pass initially so I'm assuming that this implies the need for this kind of control? I'm also unfamiliar with using the transform dialect for testing -- my understanding was that pass pipelines were generally tested using invocations of That being said, when I was writing this PR I attempted to write a test for these options. However, as someone who has limited knowledge of the There was a problem hiding this comment. These are only two knobs which make a huge difference in the transformations that happen within the pass. It just happens that they are enums and have a bunch of values. I think we should support them. 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 afraid that if we widely test all the combinations on existing tests we get stuck with failures.
Well, for better or worse, quite a few tests in the Vector dialect were moved from pipeline to TD so they are no longer tested on the pipeline side. There was a problem hiding this comment.
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.
Yes, but there’s a comparable number of Transform Dialect Ops. Let’s be a bit more specific:
(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. Now, note that this PR adds: clEnumValN(::mlir::vector::VectorTransposeLowering::Shuffle1D, "shuffle1d",
"Lower 2-D transpose to `vector.shuffle` on 1-D vector."),So the key question is:
Absolutely! Adding new
I'm not sure I follow ... My thinking is more along the lines of adding new mlir-opt -vector-transpose-lowering=shuffle1d %s | FileCheck --check-prefix=SHUFFLE_1Dwhile still keeping the existing: // RUN: mlir-opt %s --transform-interpreter --split-input-file | FileCheck %sThe downside? @dcaballe, I am guessing that you will use this downstream? If yes, then lets merge this - we should aim to progress ASAP. Also, I am not really providing any viable alternatives. There was a problem hiding this comment. @banach-space What about the alternative of adding tests like: // RUN: mlir-opt -convert-vector-to-llvm="vector-contract-lowering=dot" %s | FileCheck %s
func.func @vector_contract(%arg0: vector<2x2xf32>, %arg1: vector<2xf32>, %arg2: vector<2xf32>) -> vector<2xf32> {
%out = vector.contract {
// .. and so on
}
// CHECK-LABEL: @vector_contract
// Insert various checks on the resulting LLVMIR hereThis does still suffer from the check-prefix explosion but does thoroughly test the flags and the effect of these options on the overall ConvertToLLVMPass pretty unambiguously. There's also no duplication with existing vector to LLVM conversion tests because these are new options for the pass. In my opinion these are the most suitable kinds of tests but do require some upfront work for me to properly understand the transformations before I can write tests for them. I've not written Vector to LLVM conversion tests before though so I would like some thoughts on whether this is worth the effort. There was a problem hiding this comment. Thanks for the suggestion!
Note, we actually do have "options" for these, see: (this is a TD Op rather than an "option" - the point is that it exposes exactly that functionality). As for test duplication, this is already tested here: llvm-project/mlir/test/Dialect/Vector/vector-contract-to-dot-transforms.mlir Lines 303 to 306 in 60cc3af This brings me back to my original point, there are effectively 3 ways to drive most (all?) transformations:
While there's plenty of duplication throughout wider LLVM, we should actively be thinking how to reduce that. |
||
| ]; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the additional include required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot, thank you
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this has been removed and then added back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it looks unused but it's actually required for this file. You can see some enums from this file are referenced in the description of the options for the
ConvertVectorToLLVMPass, to not duplicate the option descriptions. I removed it the first time by mistake.