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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes 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
----------------
dcaballe wrote:
> 1-d -> 1-D for consistency?
> 1-d -> 1-D for consistency?

Ah good spot will fix this before landing, cheers


================
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.
----------------
dcaballe wrote:
> 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?
> I would call that inner 1-D sub-vector?

Hm, it's not an inner 1-D sub-vector until it's moved, I think that would make sense for the inverse of this operation (extract) that does tile slice to 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);
----------------
dcaballe wrote:
> 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.
> 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.

Sorry I'm not sure I follow, this is low-level and emitting the canonical form for an all active mask (a constant?), it's not clear to me what the benefit of such patterns are in this case?

I should mention there has been no consideration of masking so far in the ops introduced for our first target for SME of lowering linalg.fill, now that there is a basic path established (D158619) I've starting looking into masking.


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

https://reviews.llvm.org/D157005