This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] Transfer ops: one `in_bounds` bool per memref/tensor dim
Changes PlannedPublic

Authored by springerm on Jul 13 2023, 7:27 AM.

Details

Summary

The in_bounds attribute specifies which dimensions of a vector transfer are in-bounds and which are out-of-bounds. Until now, it was expressed in terms of transfer dimensions. This was confusing because "in-bounds" is a property of shaped value indexing.

With this change, in_bounds is expressed in terms of shaped value ("source") dimensions. I.e.:

  • The number of in_bounds bools must match the rank of the shaped value (and not the rank of the vector).
  • Special rules around broadcast dimensions are no longer needed. (Previously, broadcast dimensions had to be "in-bounds".)

This change is in preparation of allowing the starting offsets to be out-of-bounds (in a future change). This rule is *not* changed yet with this revision: starting points must be in-bounds and that is verified for non-transfer dimensions. (It cannot be verified statically for transfer dimensions of size >1.)

The following examples illustrate how transfer ops are changing.

Example 1:

// previously:
%0 = vector.transfer_read %m[%i, %j], %cst {in_bounds = [true, false], permutation_map = affine_map<(d0, d1) -> (d1, d0)>} : memref<?x?xf32>, vector<5x6xf32>
// now: in_bounds is swapped because it is now expressed in terms of shaped value dimensions
%0 = vector.transfer_read %m[%i, %j], %cst {in_bounds = [false, true], permutation_map = affine_map<(d0, d1) -> (d1, d0)>} : memref<?x?xf32>, vector<5x6xf32>

Example 2:

// previously:
%0 = vector.transfer_read %t[%i, %j, %k], %cst {in_bounds = [false]} : tensor<?x?x?xf32>, vector<5xf32>
// now: There are 3 in_bounds values. The first two in_bounds must be "true" because they are non-transfer dimensions.
%0 = vector.transfer_read %t[%i, %j, %k], %cst {in_bounds = [true, true, false]} : tensor<?x?x?xf32>, vector<5xf32>

Note that in the above example, in_bounds used to be optional. In the absence of in_bounds, all dimension are considered out-of-bounds. But this would be incorrect with the new op semantics because the non-transfer dimensions would be out-of-bounds.

Depends On: D155277

Diff Detail

Event Timeline

springerm created this revision.Jul 13 2023, 7:27 AM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
springerm requested review of this revision.Jul 13 2023, 7:27 AM
Herald added a project: Restricted Project. · View Herald Transcript
springerm updated this revision to Diff 540037.Jul 13 2023, 7:43 AM

add invalid.mlir test case

dcaballe requested changes to this revision.Jul 17 2023, 11:35 PM

Hey Matthias!

Big change! This seems to be significantly changing the meaning of the in_bounds attribute and I'm not sure the new definition is a superset of the previous definition. The in_bounds attribute in xfer ops refers to the transferred elements themself. That is, at least, how it was originally defined. The starting address could be "in bounds" but the xfer operation can still read/writer out-of-bounds, which is what in_bounds is supposed to model. Example:

%0 = vector.transfer_read %t[%c2], %cst {in_bounds = [?]} : tensor<4xf32>, vector<5xf32>

Should in_bounds be true or false for this example with the new definition? According to the original definition, it should be false...

It's not totally clear either what we are trying to model with an in_bounds value per index. For example, what does the following example mean? Is it valid? Why do we care if the first index is in_bounds or not?

%5 = vector.transfer_read %0[%c2, %c3], %cst {permutation_map = #map1, in_bounds = [false, true]} : memref<?x?xf32>, vector<5xf32>

Hopefully you can help me understand the motivation. I'm not against this change but it's a big one and I'm not sure I totally understand the implications of it. Would it make sense to send an RFC?
It would also be great to learn more about what you are trying to model.

Thanks!
Diego

This revision now requires changes to proceed.Jul 17 2023, 11:35 PM
springerm planned changes to this revision.Jul 19 2023, 8:58 AM

Putting this on hold for the moment. We may not need it.