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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added inline comments.


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:430
+    The vector to tile slice operation moves a 1-D scalable vector to a slice
+    of a 2-D scalable vector tile at the given index. The type of the 1-d
+    scalable vector to be moved must match the type of the tile slice. A tile
----------------
1-d -> 1-D for consistency?


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:424
+    (2-D scalable vector) slice at the given index. The 1-D vector type must
+    match the inner vector type of the 2-D vector type that represents the
+    tile. The updated tile is returned as the result.
----------------
c-rhodes wrote:
> awarzynski wrote:
> > c-rhodes wrote:
> > > dcaballe wrote:
> > > > -> element type?
> > > > -> element type?
> > > 
> > > For a 2-d scalable vector such as `vector<[4]x[4]xi32>` is the element type `vector<[4]xi32>`? That's what I want to express here but I do struggle with these descriptions. FWIW `getElementType()` returns the scalar `i32`. Perhaps `The 1-D vector type must match the vector type of the inner dimension of the 2-D vector type`?
> > I also though that you meant the element type here, as in e.g. `i32` or `f32`.
> > 
> > I think that we need to be more explicit here:
> > 
> > > The type of the 1-d scalable vector to be moved must match the type of the tile slice (note that 1 slice is effectively 1 row in a virtual tile).
> > 
> > WDYT?
> > I also though that you meant the element type here, as in e.g. `i32` or `f32`.
> > 
> > I think that we need to be more explicit here:
> > 
> > > The type of the 1-d scalable vector to be moved must match the type of the tile slice (note that 1 slice is effectively 1 row in a virtual tile).
> > 
> > WDYT?
> 
> I've updated the description along these lines, hope it makes more sense now, appreciate the input!
I would call that inner 1-D sub-vector?


================
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);
----------------
c-rhodes wrote:
> 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.
Well, if this is expected to run before some of these "higher" level vector ops are lowered, I would make sure we generate those. We have patterns that are looking specifically for those and are able to understand if they are all-true/all-false masks, etc. We don't have mask patterns looking at constant ops.


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

https://reviews.llvm.org/D157005