[PATCH] D72808: [mlir] [VectorOps] Lowering of vector.extract_slices to LLVM IR

[Aart Bik via Phabricator via llvm-commits]

aartbik marked 13 inline comments as done.
aartbik added inline comments.


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:900
 
+class VectorTupleGetOpConversion : public LLVMOpLowering {
+public:
----------------
nicolasvasilache wrote:
> This looks more like a rewrite / folding pattern than a conversion to me (i.e. the types don't change).
> I would prob go for a folding pattern in VectorOps.cpp but we also need to make sure it is accessible and  
> In any case please document with a before / after IR example.
Agreed in isolation, but since we introduce vector.tuple in other rewritings, I really need a ConversionPattern here, so that I see the newly introduced operands.


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:901
+class VectorTupleGetOpConversion : public LLVMOpLowering {
+public:
+  explicit VectorTupleGetOpConversion(MLIRContext *context,
----------------
nicolasvasilache wrote:
> Well on second inspection you already have that in the VectorToVectorOps conversion.
> So please don't duplicate here and just make the vectorToVector canonicalization patterns "insert" the "foldingPatterns".
> 
> Structurally this is also an indication that VectorToLLVM should start including all the VectorToVector rewrites so that things compose properly.
I avoided the duplication for now by just having the lowering change. Once we have a proper vector2vector rewriting, we can move it into folding again and populate.


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:930
+                  ConversionPatternRewriter &rewriter) const override {
+    // TODO(ajcbik): bit of a hack for now; if not all uses are
+    //               folded into a matching TupleGetOp, erasing
----------------
nicolasvasilache wrote:
> Yes that is hacky indeed :)
> 
> You could just return failure if the uses are not empty:
> ```
> if (!op->getResult()->use_empty())
>   return matchFailure();
> ```
> 
> The pattern will fail to apply until all canonicalizations occured at which point it will apply.
> This will also error out gracefully by using the pattern rewrite infrastructure and fail a "full conversion" if things don't canonicalize.
Two complications I added to the comments. For newly introduced tuples, neither the op nor the op.getResult use_empty call is up-to-date (it is always empty). However, since this is a lowering pass, such new nodes need to be legalized anyway, so removing them is the right way. We still could assert if our rules somehow introduce a tuple we don't consume.


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:991
 
+/// Progressive lowering of ExtractSlicesOp to tuple of StridedSliceOp.
+class VectorExtractSlicesOpConversion
----------------
nicolasvasilache wrote:
> Very cool, yay intuition :) !
> 
> Same comments as above apply still: bit more doc with before/after IR.
> I may not have set a good example myself here in the past, sorry about that! 
> As the number of patterns grow we want short and intuitive examples to just get what these do without going deep in code.
> 
> Also, this should go through VectorToVector patterns and be included in the VectorToLLVM patterns (e.g. what if the target is not LLVM, some of our friends at Xilinx seem to be in this case for example). 
It sounds like we really need to move some of this into a vector to vector pass. That will also avoid some of the complications above.


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:1015
+
+    // Compute the strides between slices in each dimension.
+    SmallVector<int64_t, 4> sliceStrides(rank);
----------------
nicolasvasilache wrote:
> Strides is probably become overloaded, maybe we should rename `strided_slice` and `insert_strided_slice` as Insert/ExtractSlice and these ops as Insert/ExtractSliceTuple or something along those lines?
> In particular the `stride` attribute is not really a stride but a `step`.
> 
> In any case, we should have help functions for computing this.
> Could you please reuse/refactor/adapt the first functions in VectorTransforms.cpp and move the to VectorOps/Utils.h as appropriate?
> Thanks!
I see similar TODOs elsewhere, so let's postpone that into one CL later.


================
Comment at: mlir/lib/Dialect/VectorOps/VectorOps.cpp:1701
+
+void TupleGetOp::getCanonicalizationPatterns(OwningRewritePatternList &results,
+                                             MLIRContext *context) {
----------------
nicolasvasilache wrote:
> This can be called directly from other getXXXPatterns that want to use it, no need to duplicate the pattern.
For now I do everything in lowering, leaving proper interaction with folding/canonicalization for later.


================
Comment at: mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir:532
+}
+// CHECK-LABEL: extract_strides(%arg0: !llvm<"[3 x <3 x float>]">)
+//      CHECK: %[[s0:.*]] = llvm.mlir.constant(dense<0.000000e+00> : vector<1x1xf32>) : !llvm<"[1 x <1 x float>]">
----------------
nicolasvasilache wrote:
> This should be made into a VectorToVector test, that more directly tests "extract_slices" -> "strided_slices" + the removal of the tuple op.
> Atm it tests too much at a distance and involves multiple patterns coming together.
> Testing those patterns coming together could be done in a separate test pass where canonicalization, DCE and pattern interaction would kick in too and many things would get simplified.
I agree. Once we have a proper vector to vector pass, we should test this (and others) in intermediate stages too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72808