[PATCH] D80375: [mlir][Vector] Add vector contraction to outerproduct lowering

[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/Vector/VectorTransforms.h:96
+  /// Options to control the vector patterns.
+  vector::VectorTransformsOptions vectorTransformsOptions;
+
----------------
fields should follow methods (as you did below)

also , can this be a private field?


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorTransforms.h:98
+
+  LogicalResult match(vector::ContractionOp op) const override;
+  void rewrite(vector::ContractionOp op,
----------------
just curious, is the move away from matchAndRewrite in favor of match and rewrite intentional?


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorTransforms.h:133
+  /// Options to control the vector patterns.
+  vector::VectorTransformsOptions vectorTransformsOptions;
+};
----------------
probably private?


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorTransforms.h:160
+  /// Options to control the vector patterns.
+  vector::VectorTransformsOptions vectorTransformsOptions;
+
----------------
fields after methods, probably private


================
Comment at: mlir/lib/Dialect/Vector/VectorTransforms.cpp:1322
 
+// Helper to find an index in an affine map.
+static Optional<int64_t> getResultIndex(AffineMap map, int64_t index) {
----------------
now these are static helpers, should we move them to the top of the file
(I have seen both styles, keep methods close to use, or move them to top)

I personally like at the top a bit better since it "encourages" other classes to start using them too, if they appear in the middle, they are less visible...

just my 2 cents


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80375