[mlir][vector] Make the in_bounds attribute mandatory #97049
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
banach-space
force-pushed
the
andrzej/make_in_bounds_mandatory
branch
2 times, most recently
from
e7848f5
to
925fd02
Compare
dcaballe
reviewed
Jul 1, 2024
banach-space
force-pushed
the
andrzej/make_in_bounds_mandatory
branch
from
925fd02
to
01bf155
Compare
banach-space
requested review from
grypp,
aartbik,
PeimingLiu,
yinying-lisa-li,
matthias-springer,
hanhanW and
nicolasvasilache
as code owners
llvmbot
added
mlir:core
|
@llvm/pr-subscribers-mlir-affine @llvm/pr-subscribers-mlir Author: Andrzej Warzyński (banach-space) ChangesMakes the Updates in testsDefault
|
|
For context - this is a follow-up for #96031. Sadly, there's a lot of churn in the tests. To make reviewing easier - *test and code updates have been split into 2 separate commits. The updates in the tests are quite dense. Ideally we should avoid that, but I struggled to find a solution that would work. I might be missing something obvious - suggestions are very welcome. |
joker-eph
reviewed
Jul 2, 2024
| @@ -1363,7 +1363,7 @@ def Vector_TransferReadOp : | |||
| AffineMapAttr:$permutation_map, | |||
| AnyType:$padding, | |||
| Optional<VectorOf<[I1]>>:$mask, | |||
| OptionalAttr<BoolArrayAttr>:$in_bounds)>, | |||
| BoolArrayAttr:$in_bounds)>, | |||
There was a problem hiding this comment.
Can we use a DenseBoolArrayAttr?
There was a problem hiding this comment.
Ping on this thread? (maybe you missed it)
(It might be something to address in a separate PR as well)
There was a problem hiding this comment.
I did try this briefly, but hit build errors :(
Yes, I'd rather leave this for a separate PR.
|
We're lacking a motivation in the description, mostly around why we wouldn't want a default value? |
Added extra paragraph to the summary.
I found a way to keep the default value. I will be updating this shortly. |
banach-space
force-pushed
the
andrzej/make_in_bounds_mandatory
branch
from
01bf155
to
b46f737
Compare
|
Summary of updates:
|
banach-space
force-pushed
the
andrzej/make_in_bounds_mandatory
branch
from
b46f737
to
b24edbf
Compare
joker-eph
reviewed
Jul 5, 2024
mlir/test/Dialect/Vector/vector-transfer-to-vector-load-store.mlir
Outdated
Show resolved
Hide resolved
banach-space
requested review from
ftynse,
makslevental and
stellaraccident
as code owners
|
✅ With the latest revision this PR passed the Python code formatter. |
banach-space
force-pushed
the
andrzej/make_in_bounds_mandatory
branch
2 times, most recently
from
b94af64
to
89af514
Compare
joker-eph
reviewed
Jul 7, 2024
| ) | ||
| vector.TransferReadOp( | ||
| vector_type, A, [zero, zero], identity_map_attr, padding |
There was a problem hiding this comment.
We're losing the default in Python? Can this be recovered somehow?
There was a problem hiding this comment.
Can this be recovered somehow?
I don't think so.
From a quick scan, the Python bindings are build based on what's in ODS. However, the default value for in_bounds is implemented by customising C++:
- the Op parsers (updated here), and
- the Op builders (already flexible enough).
IIUC, such C++ customisation are not visible/available through Python bindings.
This was referenced Jul 10, 2024
dcaballe
approved these changes
Jul 11, 2024
mlir/include/mlir/IR/AffineMap.h
Outdated
| /// * (d0, d1, d2) -> (0, d1) gives [0] | ||
| /// * (d0, d1, d2) -> (d2, d1) gives [] | ||
| /// * (d0, d1, d2, d4) -> (d0, 0, d1, 0) gives [1, 3] | ||
| SmallVector<unsigned> getBroadcastDims() const; |
There was a problem hiding this comment.
Have you looked at isMinorIdentityWithBroadcasting
There was a problem hiding this comment.
Yes - that was my source of inspiration :) It's just below getBroadcastDims (though I am about to move it a bit).
Note, isMinorIdentityWithBroadcasting wouldn't work as maps in xfer_read/xfer_write can be permutations. I also tried re-using getBroadcastDims inside isMinorIdentityWithBroadcasting, but that didn't feel right (perhaps isMinorIdentityWithBroadcasting should be split?).
| @@ -1223,8 +1223,19 @@ static Operation *vectorizeAffineLoad(AffineLoadOp loadOp, | |||
| LLVM_DEBUG(dbgs() << "\n[early-vect]+++++ permutationMap: "); | |||
| LLVM_DEBUG(permutationMap.print(dbgs())); | |||
|
|
|||
| // Make sure that the in_bounds attribute corresponding to a broadcast dim | |||
| // is set to `true` - that's required by the xfer Op. | |||
There was a problem hiding this comment.
Nothing for this PR but this is still a constraint that seems unnecessary and makes this complicated, as we can see here.
There was a problem hiding this comment.
Agreed. Expect a PR to remove this shortly :) (~next week)
hanhanW
reviewed
Jul 11, 2024
| out-of-bounds. A `vector.transfer_read` can be lowered to a simple load if | ||
| all dimensions are specified to be within bounds and no `mask` was | ||
| specified. Note that non-vector dimensions *must* always be in-bounds. | ||
| Non-vector and broadcast dimensions *must* always be in-bounds. The |
There was a problem hiding this comment.
dumb question: what does Non-vector mean here?
There was a problem hiding this comment.
I also don't know :) That note was introduced by @matthias-springer in https://reviews.llvm.org/D155719. Matthias, do you remember what cases that was referring to?
Makes the `in_bounds` attribute for vector.transfer_read and
vector.transfer_write Ops mandatory. In addition, makes the Asm printer
always print this attribute - tests are updated accordingly.
1. Updates in tests - default `in_bounds` value
Originally, most tests would skip the `in_bounds` attribute - this was
equivalent to setting all values to `false` [1]. With this change, this
has to be done explicitly when writing a test. Note, especially when
reviewing this change, that the vast majority of newly inserted
`in_bounds` attributes are set to `false` to preserve the original
semantics of the tests.
There is only one exception - for broadcast dimensions the newly
inserted `in_bounds` attribute is set to `true`. As per [2]:
```
vector.transfer_read op requires broadcast dimensions to be in-bounds
```
This matches the original semantics:
* the `in_bounds` attribute in the context of broadcast dimensions
would only be checked when present,
* the verifier wasn't aware of the default value set in [1],
This means that effectively, the attribute was set to `false` even for
broadcast dims, but the verifier wasn't aware of that. This change makes
that behaviour more explicit by setting the attribute to `true` for
broadcast dims. In all other cases, the attribute is set to `false` - if
that's not the case, consider that as a typo.
2. Updates in tests - 0-D vectors
Reading and writing to/from 0D vectors also requires the `in_bounds`
attribute. In this case, the attribute has to be empty:
```mlir
vector.transfer_write %5, %m1[] {in_bounds=[]} : vector<f32>, memref<f32>
```
3. Updates in tests - CHECK lines
With this PR, the `in_bounds` attribute is always print. This required
updating the `CHECK` lines that previously assumed that the attribute
would be skipped. To keep this type of changes simple, I've only added
`{{.*}}` to make sure that tests pass.
4. Changes in "Vectorization.cpp"
The following patterns are updated to explicitly set the `in_bounds`
attribute to `false`:
* `LinalgCopyVTRForwardingPattern` and `LinalgCopyVTWForwardingPattern`
5. Changes in "SuperVectorize.cpp" and "Vectorization.cpp"
The updates in `vectorizeAffineLoad` (SuperVectorize.cpp) and
`vectorizeAsLinalgGeneric` (Vectorization.cpp) are introduced to make
sure that xfer Ops created by these vectorisers set the dimension
corresponding to broadcast dims as "in bounds". Otherwise, the Op
verifier would fail.
Note that there is no mechanism to verify whether the corresponding
memory access are indeed in bounds. Previously, when `in_bounds` was
optional, the verification would skip checking the attribute if it
wasn't present. However, it would default to `false` in other places.
Put differently, this change does not change the existing behaviour, it
merely makes it more explicit.
[1] https://github.com/llvm/llvm-project/blob/4145ad2bac4bb99d5034d60c74bb2789f6c6e802/mlir/include/mlir/Interfaces/VectorInterfaces.td#L243-L246
[2] https://mlir.llvm.org/docs/Dialects/Vector/#vectortransfer_read-vectortransferreadop
At the moment, the in_bounds attribute has two confusing/contradicting
properties:
1. It is both optional _and_ has an effective default-value.
2. The default value is "out-of-bounds" for non-broadcast dims, and
"in-bounds" for broadcast dims.
(see the `isDimInBounds` vector interface method for an example of this
"default" behaviour [1]).
This PR aims to clarify the logic surrounding the `in_bounds` attribute
by:
* making the attribute it mandatory (i.e. it is always present),
* always setting the default value to "out of bounds" (that's
consistent with the current behaviour for the most common cases).
1. Broadcast dimensions in tests
As per [2], the broadcast dimensions requires the corresponding
`in_bounds` attribute to be `true`:
```
vector.transfer_read op requires broadcast dimensions to be in-bounds
```
The changes in this PR mean that we can no longer rely on the
default value in cases like the following (dim 0 is a broadcast dim):
```mlir
%read = vector.transfer_read %A[%base1, %base2], %f, %mask
{permutation_map = affine_map<(d0, d1) -> (0, d1)>} :
memref<?x?xf32>, vector<4x9xf32>
```
Instead, the broadcast dimension has to explicitly be marked as "in
bounds:
```mlir
%read = vector.transfer_read %A[%base1, %base2], %f, %mask
{in_bounds = [true, false], permutation_map = affine_map<(d0, d1) -> (0, d1)>} :
memref<?x?xf32>, vector<4x9xf32>
```
All tests with broadcast dims are updated accordingly.
2. Changes in "SuperVectorize.cpp" and "Vectorization.cpp"
The following patterns in "Vectorization.cpp" are updated to explicitly
set the `in_bounds` attribute to `false`:
* `LinalgCopyVTRForwardingPattern` and `LinalgCopyVTWForwardingPattern`
Also, `vectorizeAffineLoad` (from "SuperVectorize.cpp") and
`vectorizeAsLinalgGeneric` (from "Vectorization.cpp") are updated to
make sure that xfer Ops created by these hooks set the dimension
corresponding to broadcast dims as "in bounds". Otherwise, the Op
verifier would complain
Note that there is no mechanism to verify whether the corresponding
memory access are indeed in bounds. Still, this is consistent with the
current behaviour where the broadcast dim would be implicitly assumed
to be "in bounds".
[1] https://github.com/llvm/llvm-project/blob/4145ad2bac4bb99d5034d60c74bb2789f6c6e802/mlir/include/mlir/Interfaces/VectorInterfaces.td#L243-L246
[2] https://mlir.llvm.org/docs/Dialects/Vector/#vectortransfer_read-vectortransferreadop
Elide `in_bounds = []`
…atory Update docs
…te mandatory Address comment from Diego and Han-Chung
banach-space
force-pushed
the
andrzej/make_in_bounds_mandatory
branch
from
5f82c64
to
1bc7464
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At the moment, the in_bounds attribute has two confusing/contradicting
properties:
"in-bounds" for broadcast dims.
(see the
isDimInBoundsvector interface method for an example of this"default" behaviour [1]).
This PR aims to clarify the logic surrounding the
in_boundsattributeby:
consistent with the current behaviour for the most common cases).
Broadcast dimensions in tests
As per [2], the broadcast dimensions requires the corresponding
in_boundsattribute to betrue:The changes in this PR mean that we can no longer rely on the
default value in cases like the following (dim 0 is a broadcast dim):
Instead, the broadcast dimension has to explicitly be marked as "in
bounds:
All tests with broadcast dims are updated accordingly.
Changes in "SuperVectorize.cpp" and "Vectorization.cpp"
The following patterns in "Vectorization.cpp" are updated to explicitly
set the
in_boundsattribute tofalse:LinalgCopyVTRForwardingPatternandLinalgCopyVTWForwardingPatternAlso,
vectorizeAffineLoad(from "SuperVectorize.cpp") andvectorizeAsLinalgGeneric(from "Vectorization.cpp") are updated tomake sure that xfer Ops created by these hooks set the dimension
corresponding to broadcast dims as "in bounds". Otherwise, the Op
verifier would complain
Note that there is no mechanism to verify whether the corresponding
memory access are indeed in bounds. Still, this is consistent with the
current behaviour where the broadcast dim would be implicitly assumed
to be "in bounds".
[1]
llvm-project/mlir/include/mlir/Interfaces/VectorInterfaces.td
Lines 243 to 246 in 4145ad2
[2] https://mlir.llvm.org/docs/Dialects/Vector/#vectortransfer_read-vectortransferreadop