[PATCH] D76266: [mlir][Vector] Mostly-NFC - Restructure options for lowering to LLVM Matrix Intrinsics

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache marked 6 inline comments as done.
nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/Dialect/VectorOps/VectorOps.h:28
+struct VectorTransformsOptions {
+  bool lowerToLLVMMatrixIntrinsics = false;
+};
----------------
aartbik wrote:
> provide convenience constructors for this
> 
> (one bool is simple but if we add more that may come in handy?)
What benefit do constructors bring in this instance?
A simple struct with public members is the simplest thing in my book.

Once multiple options are available and they can be logically grouped and given a meaningful name we can provide static constructors that configure a set of options.

We can also provide chaining semantics to have something like: `VectorTransformsOptions().setX(true).setY(3)`

All these will make sense in the future but are premature until we have at least 3-5 flags + behaviors that must compound. 


================
Comment at: mlir/test/lib/Transforms/TestVectorTransforms.cpp:28
+    llvm::cl::desc("Lower vector.contract to llvm.intr.matrix.multiply"),
+    llvm::cl::init(false), llvm::cl::cat(clOptionsCategory));
+
----------------
mehdi_amini wrote:
> Can you use pass options instead?
very cool! Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76266/new/

https://reviews.llvm.org/D76266