This is an archive of the discontinued LLVM Phabricator instance.

Differential D86197

[mlir][VectorToSCF] Bug in TransferRead lowering fixed
ClosedPublic

Authored by limo1996 on Aug 19 2020, 12:06 AM.

Details

Reviewers
ftynse
aartbik
nicolasvasilache
Summary

If Memref has rank > 1 this pass emits N-1 loops around
TransferRead op and transforms the op itself to 1D read. Since vectors
must have static shape while memrefs don't the pass emits if condition
to prevent out of bounds accesses in case some memref dimension is smaller
than the corresponding dimension of targeted vector. This logic is fine
but authors forgot to apply permutation_map on loops upper bounds and
thus if condition compares induction variable to incorrect loop upper bound
(dimension of the memref) in case permutation_map is not identity map.
This commit aims to fix that.

Diff Detail

Event Timeline

limo1996 created this revision.Aug 19 2020, 12:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 12:06 AM
Herald added subscribers: msifontes, jurahul, Kayjukh and 11 others. · View Herald Transcript
limo1996 requested review of this revision.Aug 19 2020, 12:06 AM
Herald added a subscriber: stephenneuendorffer. · View Herald TranscriptAug 19 2020, 12:06 AM
Harbormaster completed remote builds in B68858: Diff 286489.Aug 19 2020, 12:23 AM
limo1996 updated this revision to Diff 286494.Aug 19 2020, 12:29 AM

update

Harbormaster completed remote builds in B68859: Diff 286494.Aug 19 2020, 12:44 AM
ftynse added a comment.Aug 19 2020, 1:05 AM

Can we have a test for this?

nicolasvasilache requested changes to this revision.Aug 19 2020, 1:35 AM

There are a few different places where patterns apply where we make the test that the permutation map is identity and that the rest is not yet supported.
Please update such places too and add tests to extend support more generally.

Thanks for extending this functionality!

This revision now requires changes to proceed.Aug 19 2020, 1:35 AM
limo1996 updated this revision to Diff 286527.Aug 19 2020, 3:47 AM

added test

limo1996 added a comment.Aug 19 2020, 3:48 AM
In D86197#2225657, @ftynse wrote:

Can we have a test for this?

Yes! I added one.

limo1996 added a comment.Aug 19 2020, 4:01 AM
In D86197#2225689, @nicolasvasilache wrote:

There are a few different places where patterns apply where we make the test that the permutation map is identity and that the rest is not yet supported.
Please update such places too and add tests to extend support more generally.

Thanks for extending this functionality!

I did not quite get the comment probably because it is too generic. I don't see any checks for isIdentity in the vector-to-scf pass only for isMinorIdentity which happens once for TransferRead and once for TransferWrite. I am technically also not extending the functionality but rather fixing what should be supported. I can fix it in other places as well but can you be please more concreate if those places are in this lowering pass or also in some other one? Or you meant fixing it for TransferWrite as well? Thanks.

Harbormaster completed remote builds in B68878: Diff 286527.Aug 19 2020, 4:02 AM
nicolasvasilache resigned from this revision.Aug 19 2020, 5:13 AM

I was under the impression that you were implementing the more general case but indeed not.
Then please just make sure the writes behave properly too, thanks!

Resigning as a reviewer to unblock as I am OOO until early Sept.

Herald added a subscriber: nicolasvasilache. · View Herald TranscriptAug 19 2020, 5:13 AM
limo1996 updated this revision to Diff 286548.Aug 19 2020, 6:27 AM

added test for transfer_write

limo1996 added a comment.Aug 19 2020, 6:29 AM
In D86197#2225972, @nicolasvasilache wrote:

I was under the impression that you were implementing the more general case but indeed not.
Then please just make sure the writes behave properly too, thanks!

Resigning as a reviewer to unblock as I am OOO until early Sept.

Ok. I found out that the change I made works fine also for vector.transfer_write and therefore I just added a test case for that.

Harbormaster completed remote builds in B68887: Diff 286548.Aug 19 2020, 6:45 AM
ftynse accepted this revision.Aug 19 2020, 6:47 AM

Thanks for the fix!

mlir/test/Conversion/VectorToSCF/vector-to-loops.mlir
415

I don't think we care that this is on the next line, so I'd drop -NEXT from checks here and below.

This revision is now accepted and ready to land.Aug 19 2020, 6:47 AM
limo1996 updated this revision to Diff 286565.Aug 19 2020, 8:14 AM

CHECK-NEXT -> CHECK

Harbormaster completed remote builds in B68898: Diff 286565.Aug 19 2020, 8:29 AM
limo1996 marked an inline comment as done.Aug 19 2020, 8:33 AM
limo1996 closed this revision.Aug 31 2020, 7:53 AM

Landed as 8dace28f92056a04e02d913684d3f448ee1f295a

Revision Contents

PathSize
mlir/
lib/
Conversion/
VectorToSCF/
6 lines
test/
Conversion/
VectorToSCF/
57 lines

Diff 286565

mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp

Show All 10 Lines
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include <type_traits> #include <type_traits>
#include "mlir/Conversion/VectorToSCF/VectorToSCF.h" #include "mlir/Conversion/VectorToSCF/VectorToSCF.h"
#include "../PassDetail.h" #include "../PassDetail.h"
#include "mlir/Dialect/Affine/EDSC/Intrinsics.h" #include "mlir/Dialect/Affine/EDSC/Intrinsics.h"
#include "mlir/Dialect/Linalg/Utils/Utils.h"
#include "mlir/Dialect/SCF/EDSC/Builders.h" #include "mlir/Dialect/SCF/EDSC/Builders.h"
#include "mlir/Dialect/SCF/EDSC/Intrinsics.h" #include "mlir/Dialect/SCF/EDSC/Intrinsics.h"
#include "mlir/Dialect/StandardOps/EDSC/Intrinsics.h" #include "mlir/Dialect/StandardOps/EDSC/Intrinsics.h"
#include "mlir/Dialect/Vector/EDSC/Intrinsics.h" #include "mlir/Dialect/Vector/EDSC/Intrinsics.h"
#include "mlir/Dialect/Vector/VectorOps.h" #include "mlir/Dialect/Vector/VectorOps.h"
#include "mlir/Dialect/Vector/VectorUtils.h" #include "mlir/Dialect/Vector/VectorUtils.h"
#include "mlir/IR/AffineExpr.h" #include "mlir/IR/AffineExpr.h"
#include "mlir/IR/AffineMap.h" #include "mlir/IR/AffineMap.h"
▲ Show 20 Lines • Show All 171 Lines • ▼ Show 20 Lines
template <typename ConcreteOp> template <typename ConcreteOp>
Value NDTransferOpHelper<ConcreteOp>::emitInBoundsCondition( Value NDTransferOpHelper<ConcreteOp>::emitInBoundsCondition(
ValueRange majorIvs, ValueRange majorOffsets, ValueRange majorIvs, ValueRange majorOffsets,
MemRefBoundsCapture &memrefBounds, MemRefBoundsCapture &memrefBounds,
SmallVectorImpl<Value> &majorIvsPlusOffsets) { SmallVectorImpl<Value> &majorIvsPlusOffsets) {
Value inBoundsCondition; Value inBoundsCondition;
majorIvsPlusOffsets.reserve(majorIvs.size()); majorIvsPlusOffsets.reserve(majorIvs.size());
unsigned idx = 0; unsigned idx = 0;
for (auto it : llvm::zip(majorIvs, majorOffsets, memrefBounds.getUbs())) { SmallVector<Value, 4> bounds =
linalg::applyMapToValues(rewriter, xferOp.getLoc(),
xferOp.permutation_map(), memrefBounds.getUbs());
for (auto it : llvm::zip(majorIvs, majorOffsets, bounds)) {
Value iv = std::get<0>(it), off = std::get<1>(it), ub = std::get<2>(it); Value iv = std::get<0>(it), off = std::get<1>(it), ub = std::get<2>(it);
using namespace mlir::edsc::op; using namespace mlir::edsc::op;
majorIvsPlusOffsets.push_back(iv + off); majorIvsPlusOffsets.push_back(iv + off);
if (xferOp.isMaskedDim(leadingRank + idx)) { if (xferOp.isMaskedDim(leadingRank + idx)) {
Value inBoundsCond = onTheFlyFoldSLT(majorIvsPlusOffsets.back(), ub); Value inBoundsCond = onTheFlyFoldSLT(majorIvsPlusOffsets.back(), ub);
if (inBoundsCond) if (inBoundsCond)
inBoundsCondition = (inBoundsCondition) inBoundsCondition = (inBoundsCondition)
? (inBoundsCondition && inBoundsCond) ? (inBoundsCondition && inBoundsCond)
▲ Show 20 Lines • Show All 484 Lines • Show Last 20 Lines

mlir/test/Conversion/VectorToSCF/vector-to-loops.mlir

Show First 20 Lines • Show All 394 Lines • ▼ Show 20 Linesfunc @transfer_read_simple(%A : memref<2x2xf32>) -> vector<2x2xf32> {
// FULL-UNROLL-DAG: %[[C1:.*]] = constant 1 : index // FULL-UNROLL-DAG: %[[C1:.*]] = constant 1 : index
// FULL-UNROLL: %[[V0:.*]] = vector.transfer_read %{{.*}}[%[[C0]], %[[C0]]] // FULL-UNROLL: %[[V0:.*]] = vector.transfer_read %{{.*}}[%[[C0]], %[[C0]]]
// FULL-UNROLL: %[[RES0:.*]] = vector.insert %[[V0]], %[[VC0]] [0] : vector<2xf32> into vector<2x2xf32> // FULL-UNROLL: %[[RES0:.*]] = vector.insert %[[V0]], %[[VC0]] [0] : vector<2xf32> into vector<2x2xf32>
// FULL-UNROLL: %[[V1:.*]] = vector.transfer_read %{{.*}}[%[[C1]], %[[C0]]] // FULL-UNROLL: %[[V1:.*]] = vector.transfer_read %{{.*}}[%[[C1]], %[[C0]]]
// FULL-UNROLL: %[[RES1:.*]] = vector.insert %[[V1]], %[[RES0]] [1] : vector<2xf32> into vector<2x2xf32> // FULL-UNROLL: %[[RES1:.*]] = vector.insert %[[V1]], %[[RES0]] [1] : vector<2xf32> into vector<2x2xf32>
%0 = vector.transfer_read %A[%c0, %c0], %f0 : memref<2x2xf32>, vector<2x2xf32> %0 = vector.transfer_read %A[%c0, %c0], %f0 : memref<2x2xf32>, vector<2x2xf32>
return %0 : vector<2x2xf32> return %0 : vector<2x2xf32>
} }
func @transfer_read_minor_identity(%A : memref<?x?x?x?xf32>) -> vector<3x3xf32> {
%c0 = constant 0 : index
%f0 = constant 0.0 : f32
%0 = vector.transfer_read %A[%c0, %c0, %c0, %c0], %f0
{ permutation_map = affine_map<(d0, d1, d2, d3) -> (d2, d3)> }
: memref<?x?x?x?xf32>, vector<3x3xf32>
return %0 : vector<3x3xf32>
}
// CHECK-LABEL: transfer_read_minor_identity(
// CHECK-SAME: %[[A:.*]]: memref<?x?x?x?xf32>) -> vector<3x3xf32>
// CHECK: %[[c0:.*]] = constant 0 : index
ftynseUnsubmitted
Done
ReplyInline Actions

I don't think we care that this is on the next line, so I'd drop -NEXT from checks here and below.

ftynse: I don't think we care that this is on the next line, so I'd drop `-NEXT` from checks here and…
// CHECK: %[[cst:.*]] = constant 0.000000e+00 : f32
// CHECK: %[[c2:.*]] = constant 2 : index
// CHECK: %[[cst0:.*]] = constant dense<0.000000e+00> : vector<3xf32>
// CHECK: %[[m:.*]] = alloca() {alignment = 128 : i64} : memref<3xvector<3xf32>>
// CHECK: %[[d:.*]] = dim %[[A]], %[[c2]] : memref<?x?x?x?xf32>
// CHECK: affine.for %[[arg1:.*]] = 0 to 3 {
// CHECK: %[[cmp:.*]] = cmpi "slt", %[[arg1]], %[[d]] : index
// CHECK: scf.if %[[cmp]] {
// CHECK: %[[tr:.*]] = vector.transfer_read %[[A]][%[[c0]], %[[c0]], %[[arg1]], %[[c0]]], %[[cst]] : memref<?x?x?x?xf32>, vector<3xf32>
// CHECK: store %[[tr]], %[[m]][%[[arg1]]] : memref<3xvector<3xf32>>
// CHECK: } else {
// CHECK: store %[[cst0]], %[[m]][%[[arg1]]] : memref<3xvector<3xf32>>
// CHECK: }
// CHECK: }
// CHECK: %[[cast:.*]] = vector.type_cast %[[m]] : memref<3xvector<3xf32>> to memref<vector<3x3xf32>>
// CHECK: %[[ret:.*]] = load %[[cast]][] : memref<vector<3x3xf32>>
// CHECK: return %[[ret]] : vector<3x3xf32>
func @transfer_write_minor_identity(%A : vector<3x3xf32>, %B : memref<?x?x?x?xf32>) {
%c0 = constant 0 : index
%f0 = constant 0.0 : f32
vector.transfer_write %A, %B[%c0, %c0, %c0, %c0]
{ permutation_map = affine_map<(d0, d1, d2, d3) -> (d2, d3)> }
: vector<3x3xf32>, memref<?x?x?x?xf32>
return
}
// CHECK-LABEL: transfer_write_minor_identity(
// CHECK-SAME: %[[A:.*]]: vector<3x3xf32>,
// CHECK-SAME: %[[B:.*]]: memref<?x?x?x?xf32>)
// CHECK: %[[c0:.*]] = constant 0 : index
// CHECK: %[[c2:.*]] = constant 2 : index
// CHECK: %[[m:.*]] = alloca() {alignment = 128 : i64} : memref<3xvector<3xf32>>
// CHECK: %[[cast:.*]] = vector.type_cast %[[m]] : memref<3xvector<3xf32>> to memref<vector<3x3xf32>>
// CHECK: store %[[A]], %[[cast]][] : memref<vector<3x3xf32>>
// CHECK: %[[d:.*]] = dim %[[B]], %[[c2]] : memref<?x?x?x?xf32>
// CHECK: affine.for %[[arg2:.*]] = 0 to 3 {
// CHECK: %[[cmp:.*]] = cmpi "slt", %[[arg2]], %[[d]] : index
// CHECK: scf.if %[[cmp]] {
// CHECK: %[[tmp:.*]] = load %[[m]][%[[arg2]]] : memref<3xvector<3xf32>>
// CHECK: vector.transfer_write %[[tmp]], %[[B]][%[[c0]], %[[c0]], %[[arg2]], %[[c0]]] : vector<3xf32>, memref<?x?x?x?xf32>
// CHECK: }
// CHECK: }
// CHECK: return