[PATCH] D80375: [mlir][Vector] Add vector contraction to outerproduct lowering
Aart Bik via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 21 11:54:04 PDT 2020
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
More information about the llvm-commits
mailing list