[mlir][vector] Make the in_bounds attribute mandatory #97049

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 = []`

Fix Python tests

…atory Update docs

…te mandatory Address comment from Diego and Han-Chung

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mlir][vector] Make the in_bounds attribute mandatory #97049

[mlir][vector] Make the in_bounds attribute mandatory #97049

Commits on Jul 15, 2024