[PATCH] D157005: [mlir][ArmSME] Add vector to tile slice op and lowerings

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked 5 inline comments as done.
c-rhodes added inline comments.


================
Comment at: mlir/lib/Conversion/VectorToArmSME/VectorToArmSME.cpp:137
+
+    // Create a loop that loads each ZA tile slice from memory.
+    auto step = rewriter.create<arith::ConstantIndexOp>(loc, 1);
----------------
awarzynski wrote:
> IIUC, the following block will only create the loop structure. The following operation is not created here:
> 
> > loads each ZA tile slice from memory.
> 
> That's created further down. Also, rather than "loading from memory", this is creating "move from vector to a slice", right?
> IIUC, the following block will only create the loop structure. The following operation is not created here:
> 
> > loads each ZA tile slice from memory.
> 
> That's created further down. Also, rather than "loading from memory", this is creating "move from vector to a slice", right?




================
Comment at: mlir/lib/Dialect/ArmSME/Transforms/LegalizeForLLVMExport.cpp:280-285
+    auto one = rewriter.create<arith::ConstantOp>(
+        loc, rewriter.getI1Type(),
+        rewriter.getIntegerAttr(rewriter.getI1Type(), 1));
+    auto predTy = VectorType::get(tileType.getShape()[0], rewriter.getI1Type(),
+                                  /*scalableDims=*/{true});
+    auto allActiveMask = rewriter.create<vector::SplatOp>(loc, predTy, one);
----------------
dcaballe wrote:
> We are usually creating masks with `vector.create_mask` and `vector.constant_mask` but maybe it's already too late to introduce them here. I would expect them to have been lowered in an earlier stage. Just bringing this up in case you thought about it. (I'm not asking for changes)
> We are usually creating masks with `vector.create_mask` and `vector.constant_mask` but maybe it's already too late to introduce them here. I would expect them to have been lowered in an earlier stage. Just bringing this up in case you thought about it. (I'm not asking for changes)

I hadn't thought about that, thanks for mentioning. I can't see why it wouldn't work since a vector op  (splat) is already added here, but I'm not really sure it simplifies this.


================
Comment at: mlir/test/Dialect/ArmSME/roundtrip.mlir:586
+  %c0 = arith.constant 0 : index
+  arm_sme.vector_to_tile_slice %vector, %tile, %tile_slice_index : vector<[16]xi8> into vector<[16]x[16]xi8>
+  return
----------------
awarzynski wrote:
> Could you add some invalid cases in "invalid.mlir"?
> Could you add some invalid cases in "invalid.mlir"?

Done, was also missing a type constraint that verifies 1-d vector type matches inner vector type of 2-d vector type.


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

https://reviews.llvm.org/D157005