This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] Add canonicalzation pattern for shape_cast(create_mask)
ClosedPublic

Authored by awarzynski on Aug 16 2023, 1:13 PM.

Details

Summary

This is primarily to avoid trailing unit dims:

%1 = vector.create_mask %c1, %dim_0, %c1, %c1 : vector<1x4x1x1xi1>
%2 = vector.shape_cast %1 : vector<1x4x1x1xi1> to vector<1x4xi1>

becomes:

%1 = vector.create_mask %c1, %dim_0 : vector<1x4xi1>

Diff Detail

Event Timeline

awarzynski created this revision.Aug 16 2023, 1:13 PM
Herald added a project: Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Aug 16 2023, 1:13 PM
benmxwl-arm added inline comments.Aug 18 2023, 3:58 AM
mlir/lib/Dialect/Vector/IR/VectorOps.cpp
5287–5288

This would trim vector<1x[1]x1xi128> to vector<1xi128> (same issue as in trimLeadingOneDims())

Fix (and test) how scalable vectors ar treated, add support for constant masks

dcaballe added inline comments.Aug 20 2023, 10:10 PM
mlir/lib/Dialect/Vector/IR/VectorOps.cpp
5283

Could you check if we have something like this already? We are doing something similar in quite a few places. If so, having a single utility function would be great! You can take a look at "DropUnitDims" and other patterns...

awarzynski added inline comments.Aug 21 2023, 8:27 AM
mlir/lib/Dialect/Vector/IR/VectorOps.cpp
5283

You are probably referring to trimLeadingOneDims (that I am updating in https://reviews.llvm.org/D157993).

Note that file is called "VectorDropLeadUnitDim.cpp" (_lead unit dim_) and in here I am getting rid of the _trailing_ unit dim, hence not sharing things over there. Having said that, I am implementing more patterns like this one, so we may want to group them together somewhere. I guess no need to decide just now?

5287–5288

Thanks, good catch! That's one very good reason to always test "scalable" variants too.

Switch to more consistent initialisation

benmxwl-arm accepted this revision.Aug 21 2023, 9:13 AM

LGTM! Just a minor nit/question:

mlir/lib/Dialect/Vector/IR/VectorOps.cpp
5336–5337

If any of the trailing 1 dims are false, the whole mask will be false (which is a slightly different replacement).

5344–5346

nit: semantically, should this be a success if it drops zero dims? I believe right now this will successfully replace the create_mask with an identical one.

This revision is now accepted and ready to land.Aug 21 2023, 9:13 AM
dcaballe added inline comments.Aug 21 2023, 11:07 PM
mlir/lib/Dialect/Vector/IR/VectorOps.cpp
5283

SG. I was hoping for a utility function that we can use for any combination of trailing, leading and interleaved cases...

5289

shouldn't this be part of ShapeCastOp::getCanonicalizationPatterns and not part of the transpose op?

5336

Not sure we should aim for supporting the zero case... That should probably be optimized in a different way as it means that the mask is always false...

5336–5337

I think that should never happen because all the dimensions should be at least one, right?

5344–5346

Good catch! Wouldn't this create an infinite loop? :)

5355

nit: preincrement per coding standard

c-rhodes added inline comments.Aug 22 2023, 2:24 AM
mlir/lib/Dialect/Vector/IR/VectorOps.cpp
5283

nit: a comment describing what this does particularly w.r.t. scalable would be helpful

c-rhodes added inline comments.Aug 22 2023, 6:23 AM
mlir/lib/Dialect/Vector/IR/VectorOps.cpp
5287–5288

nit: separate variables for old/new doesn't seem necessary

5299

nit: oldScalableDims, although I think it would be clearer if this is just false, and newShape = 1 above.

5345

nit: use shapeOpResTy

5362–5364

nit: I think you can just use maskDimSizes?

5368

nit: use shapeOpResTy

mlir/test/Dialect/Vector/canonicalize.mlir
2234

unused (and below)

2235

dim_1? (and below)

Thanks for all the review, an update is coming shortly :)

mlir/lib/Dialect/Vector/IR/VectorOps.cpp
5289

Sorry, forgot to move it when pushing to Phab.

5336–5337

Hm, we might be referring to different things here. Looking at this example:

%1 = vector.create_mask %c1, %dim, %c1, %c1 : vector<1x[4]x1x1xi1>

As we are only looking at _the trailing unit dimensions_ (so the trailing 1x1 in the case above), the dimension will be exactly:

  • 1 in the 1st iteration,
  • 1 in the 2nd iteration.

However, the corresponding mask value could be either 0 or 1. As Diego points out ^^^, zero case should be handled differently elsewhere, so I will just remove this comment.

5344–5346

Good point, though I am struggling to create an example that would trigger this.

mlir/test/Dialect/Vector/canonicalize.mlir
2235

Let me use dim - that will be even less confusing.

  • Rename FoldShapeCastCreateMask as ShapeCastCreateMaskFolderTrailingOneDim
  • Update tests
  • Add documetation
  • Only support cases when the mask input value is 1
c-rhodes accepted this revision.Aug 22 2023, 11:03 PM

LGTM

mlir/lib/Dialect/Vector/IR/VectorOps.cpp
4882

nit: shapeOpResTy

dcaballe accepted this revision.Aug 23 2023, 12:52 PM

Thanks for addressing the feedback :)