This is an archive of the discontinued LLVM Phabricator instance.

[mlir][VectorType] Remove `numScalableDims` from the vector type
ClosedPublic

Authored by awarzynski on Jun 21 2023, 5:35 AM.

Details

Summary

This is a follow-up of https://reviews.llvm.org/D153372 in which
numScalableDims (single integer) was effectively replaced with
isScalableDim bitmask.

This change is a part of a larger effort to enable scalable
vectorisation in Linalg. See this RFC for more context:

Depends on D153372

Diff Detail

Event Timeline

awarzynski created this revision.Jun 21 2023, 5:35 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: kuhar. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Jun 21 2023, 5:35 AM
Matt added a subscriber: Matt.Jun 21 2023, 12:42 PM
dcaballe accepted this revision.Jun 22 2023, 10:53 AM

LGTM. Only a renaming suggesting that I think it's important.

mlir/include/mlir/IR/BuiltinTypes.td
1083

nit: space after comma :)

mlir/lib/IR/BuiltinTypes.cpp
263

Should we rename the isScalableDim field to scalableDims? isScalableDim sounds like a query to know if a specific dimension is scalable

This revision is now accepted and ready to land.Jun 22 2023, 10:53 AM
awarzynski added inline comments.Jun 23 2023, 3:04 AM
mlir/lib/IR/BuiltinTypes.cpp
263

+1 Updated in https://reviews.llvm.org/D153372 (base patch for this change)

Rebase after the updates in D153372

kuhar added inline comments.Jun 24 2023, 9:13 AM
mlir/include/mlir/IR/BuiltinTypes.td
1100–1107

this can be: return llvm::is_contained(getScalableDims(), true);

1105–1107

Is this the same as checking if all values are true? If yes, I'd use return !llvm::is_contained(getScalableDims(), false);

Incorporate suggestions from @kuhar - thanks!

kuhar added inline comments.Jun 27 2023, 12:08 PM
mlir/include/mlir/IR/BuiltinTypes.td
1102–1104

Reading this definition again, should we consider a 0-d vector scalable? Logically I don't see a contradiction, but I can see that it could be confusing when one has to check that a vector is *both* scalable and has all dims scalable.

Perhaps a more useful/less error prone function would be something like isFullyScalable that combines both checks?

awarzynski added inline comments.Jun 27 2023, 2:19 PM
mlir/include/mlir/IR/BuiltinTypes.td
1102–1104

You are raising very good points.

Reading this definition again, should we consider a 0-d vector scalable?

For 0-d vectors there are no "dimensions", so there's no mechanism that would allow us to distinguish fixed-width from scalable cases. For now I'd treat them as fixed-width. Simply as the safer, better established default option. WDYT?

In general, IMHO, we need a bit more time to see how these 0-d vectors get created/processed in the context of scalable vectorisation etc. Without that it's hard to make an informed decision.

Make sure that 0-d vectors are treated as fixed-width

kuhar accepted this revision.Jun 28 2023, 12:07 AM

LGTM, thanks for the discussion!

mlir/include/mlir/IR/BuiltinTypes.td
1102–1104

SGTM

1103

nit: Use // is for local comments and end the sentence with .

This revision was landed with ongoing or failed builds.Jun 28 2023, 5:54 AM
This revision was automatically updated to reflect the committed changes.