This is an archive of the discontinued LLVM Phabricator instance.

[mlir][VectorOps] Add lowering for vector.shape_cast of scalable vectors
ClosedPublic

Authored by benmxwl-arm on Aug 30 2023, 10:48 AM.

Details

Summary

This adds a lowering similar to the general shape_cast lowering, but
instead moves elements a (scalable) subvector at a time via
vector.scalable.extract/insert. It is restricted to the case where both
the source and result vector types have a single trailing scalable
dimension (due to limitations of the insert/extract ops).

The current lowerings are now disabled for scalable vectors, as they
produce incorrect results at runtime (due to assuming a fixed number
of elements).

Examples of casts that now work:

// Flattening:
%v = vector.shape_cast %arg0 : vector<4x[8]xi8> to vector<[32]xi8>

// Un-flattening:
%v = vector.shape_cast %arg0 : vector<[8]xi32> to vector<2x1x[4]xi32>

Diff Detail

Event Timeline

benmxwl-arm created this revision.Aug 30 2023, 10:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
benmxwl-arm requested review of this revision.Aug 30 2023, 10:48 AM

Mostly makes sense - at least based on tests. I've made a few suggestions and left a few comments. In general, It would help if you documented ScalableShapeCastOpRewritePattern a bit more. Including adding comments in matchAndRewrite. Hopefully my hints will be helpful.

Thanks for working on this!

mlir/lib/Dialect/Vector/Transforms/LowerVectorShapeCast.cpp
305–348

Doxygen + short code example, pls

323–324

This comment is a bit unclear to me. How about:

%unflat = vector.shape_cast %arg0 : vector<[8]xi32> to vector<2x[1]x4xi32>

? Not that I suggest that we start generating code like this, but this pattern could in principle support this, right? It does not because it relies on vector.scalable.{insert|extract}, but that could be relaxed in the future, right?

342–343

Why "Min" rather than "Trailing"?

345–346

This is the length of the vector to be extracted?

348

Why "min"? Isn't this the total number of elements?

Also, this is ignoring "scalability", but I guess that that;s fine?

366

Rather than "extractedSubVector", could you call it "inputSubVector" (we can guess that it's been "extracted", but it's not obvious _where_ it was extracted from).

408

[nit] We know that this is testing a vector, so "VectorType" can be dropped from the name.

mlir/test/Dialect/Vector/vector-shape-cast-lowering-scalable-vectors.mlir
2

It would be good to add a "negative" test (i.e. "scalable" vectors where the scalable dim is not the trailing dim).

7

[nit] Your function names are quite long. However, we know that this file is all about "shape_cast", so you could safely drop "shape_cast" from names.
[nit] "3d_scalable_to_1d" suggests that the input is 3d scalable and the output is 1d

benmxwl-arm updated this revision to Diff 554988.EditedAug 31 2023, 5:01 AM
  • Add a bunch more tests (including a few negative tests)
  • Attempt to name tests a little better :)
  • Clear up naming and add a few explanatory comments
  • Avoid generating duplicate extracts from the source vector (when the subvector size < trailing source dim)
benmxwl-arm marked 9 inline comments as done.Aug 31 2023, 5:17 AM
benmxwl-arm added inline comments.
mlir/lib/Dialect/Vector/Transforms/LowerVectorShapeCast.cpp
323–324

I don't think this lowering (or one like this) could work for that cast.

The problems there are:

  • This lowering is part of vector-to-llvm and there's no legal (or soon to be legal) representation of vector<2x[1]x4xi32> in LLVM
  • For vector<[8]xi32> to vector<2x[1]x4xi32> you'd need extract/insert fixed vectors (vector<4xi32>) 2*vscale times, so you'd need a (runtime) loop.
  • There's (currently) no ops that could handle the require inserts/extracts.
342–343

It's min as in the minimum vector size (when vscale is 1)

I've been wanting to rewrite the (non-scalable) shape cast lowering for a while now using IndexingUtils and avoiding custom one-off incIdx like things.

Would you see an opportunity to make use of the unroll iterator introduced here: https://reviews.llvm.org/D150000 ?

It seems to me that you could use a zip with the above iterator and you have 2 cases:

  1. if shape_cast source.scalable_size == shape_cast dest.scalable_size then just vector.extract from iterator_source.drop_back and vector.insert into iterator_dest.drop_back
  2. otherwise same as 1 + an extra: a. vector.scalable.extract of iterator_source.take_back() if shape_cast source.scalable_size > shape_cast dest.scalable_size; else b. vector.scalable.insert of iterator_dest.take_back() if shape_cast source.scalable_size < shape_cast dest.scalable_size;

Bonus points for also doing the non-scalable with a similar algorithm and avoiding the custom for loop and the progressive decomposition when we can really just do a zip (but of course not necessary since your PR is orthogonal).

Would that make sense ?

awarzynski accepted this revision.Sep 6 2023, 12:50 AM

I've been wanting to rewrite the (non-scalable) shape cast lowering for a while now using IndexingUtils and avoiding custom one-off incIdx like things.

Would you see an opportunity to make use of the unroll iterator introduced here: https://reviews.llvm.org/D150000 ?

It seems to me that you could use a zip with the above iterator and you have 2 cases:

  1. if shape_cast source.scalable_size == shape_cast dest.scalable_size then just vector.extract from iterator_source.drop_back and vector.insert into iterator_dest.drop_back
  2. otherwise same as 1 + an extra: a. vector.scalable.extract of iterator_source.take_back() if shape_cast source.scalable_size > shape_cast dest.scalable_size; else b. vector.scalable.insert of iterator_dest.take_back() if shape_cast source.scalable_size < shape_cast dest.scalable_size;

Bonus points for also doing the non-scalable with a similar algorithm and avoiding the custom for loop and the progressive decomposition when we can really just do a zip (but of course not necessary since your PR is orthogonal).

Would that make sense ?

+1 to using IndexingUtils if that's possible. However, given that https://reviews.llvm.org/D150000 hasn't landed yet, would it be OK to merge this one as is and try to refactor shortly after? @nicolasvasilache ? That would unblock a few things for us.

@benmxwl-arm Approving as is - LGTM! But please wait for Nicolas to confirm before landing (or try to refactor if D150000 lands).

mlir/lib/Dialect/Vector/Transforms/LowerVectorShapeCast.cpp
368

[nit]

383
398

[nit]

414
This revision is now accepted and ready to land.Sep 6 2023, 12:50 AM
benmxwl-arm marked 2 inline comments as done.EditedSep 6 2023, 5:04 AM

I gave https://reviews.llvm.org/D150000 a quick try (and found that the patch currently has a few simple merge conflicts).
A bigger issue is the new TileOffsetRangeIterator fails to compile if passed to llvm::zip(). It seems like it's missing an implementation of std::iterator_traits.

no type named 'iterator_category' in 'std::iterator_traits<mlir::detail::TileOffsetRangeIterator<long>>'

I tried adding a basic implementation, but that leads to more issues cropping up like missing a difference_type (then the same issues for llvm::detail::zip_common<llvm::detail::zip_shortest<mlir::detail::TileOffsetRangeIterator... )

I gave https://reviews.llvm.org/D150000 a quick try (and found that the patch currently has a few simple merge conflicts).
A bigger issue is the new TileOffsetRangeIterator fails to compile if passed to llvm::zip(). It seems like it's missing an implementation of std::iterator_traits.

no type named 'iterator_category' in 'std::iterator_traits<mlir::detail::TileOffsetRangeIterator<long>>'

I tried adding a basic implementation, but that leads to more issues cropping up like missing a difference_type (then the same issues for llvm::detail::zip_common<llvm::detail::zip_shortest<mlir::detail::TileOffsetRangeIterator... )

Ok, sorry to have sent you on a wild goose chase, I'll try to land https://reviews.llvm.org/D150000 myself soon as I do not see recent activity there.
Let's proceed with this for now and add a few TODOs.

nicolasvasilache accepted this revision.Sep 7 2023, 1:03 AM
This revision was landed with ongoing or failed builds.Sep 7 2023, 9:00 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the notes above regarding D150000. I'll try to fix those today and land it