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

[Aart Bik via Phabricator via llvm-commits]

aartbik accepted this revision.
aartbik added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/include/mlir/Dialect/VectorOps/VectorOps.h:28
+struct VectorTransformsOptions {
+  bool lowerToLLVMMatrixIntrinsics = false;
+};
----------------
nicolasvasilache wrote:
> 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. 
Well, this was just a vague forward looking idea; with proper enums a constructor could read something like this

VectorTransformOpions(ENABLE_LOWER_TO_LLVM_MATRIX, USE_THIS_AND_THAT) 

when passing it into a method without the need for a chain of setters. But given that we have one bool, the point is moot :-)


================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:956
+      // In cases where matrices are degenerate, scalarization issues occur in
+      // the backend. Avoid all LLVM scalarization issues for now.
+      // For more details, see: https://bugs.llvm.org/show_bug.cgi?id=45227 and
----------------
if this is really a for now, consider adding a TODO in addition to the bug links


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76266