This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Vector] Provide progressive lowering of masked n-D vector transfers
ClosedPublic

Authored by nicolasvasilache on Apr 28 2020, 8:09 PM.

Details

Summary

This revision allows masked vector transfers with m-D buffers and n-D vectors to
progressively lower to m-D buffer and 1-D vector transfers.

For a vector.transfer_read, assuming a memref<leading_dims) x (major_dims) x (minor_dims) x type> and a vector<(minor_dims) x type> are involved in the transfer, this generates pseudo-IR resembling:

if (any_of(%ivs_major + %offsets, <, major_dims)) {
  %v = vector_transfer_read(
    {%offsets_leading, %ivs_major + %offsets_major, %offsets_minor},
     %ivs_minor):
    memref<leading_dims) x (major_dims) x (minor_dims) x type>,
    vector<(minor_dims) x type>;
} else {
  %v = splat(vector<(minor_dims) x type>, %fill)
}

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
rriddle resigned from this revision.Apr 28 2020, 8:14 PM
rriddle added inline comments.
mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp
71

///?

137

Why are you building a vector here? Is it because AffineLoopNestBuilder takes an ArrayRef<Value> instead of ValueRange?

159

inbounds &= inBounds2;?

ftynse accepted this revision.Apr 29 2020, 8:36 AM
ftynse added inline comments.
mlir/include/mlir/EDSC/Builders.h
122–123

This comment needs an update

129

This will crash if a block has an operation, which is not known to be a terminator (which may happen in rewrite patterns). I would prefer a more cautious way: get the last operation, check if it is a known terminator, and if so, update the insertion point.

205–206

Typo: "thes"

mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp
62

Here and below + in the commit description, the type spelling memref<leading_dims) x (major_dims) x (minor_dims) x type> looks broken to me.

94

Is this memref always in the default address space? What if the memref we are transferring to/from is in the non-default address space?

98

plz document here and below

101

Nit: this "1." without any continuation in the list looks fishy

104

ValueRange?

153

Nit: please add /*name=*/ annotations to differentiate which 1 corresponds to what

184

Maybe use minorRank instead of hardcoded 1 here?

586–587

Why rename Conversion to Lowering? We decided to use the former instead of the latter....

mlir/lib/Dialect/Vector/VectorOps.cpp
1491

Should we also carry over the address space of the original memref type?

This revision is now accepted and ready to land.Apr 29 2020, 8:36 AM
nicolasvasilache marked 17 inline comments as done.Apr 29 2020, 6:12 PM
nicolasvasilache added inline comments.
mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp
137

It's because the first argument of AffineLoopNestBuilder is a MutableArrayRef<Value> which captures the induction variables so they can be reused.

159

This would require Value::operator&=(Value) I believe ?

nicolasvasilache marked 2 inline comments as done.Apr 29 2020, 6:12 PM
nicolasvasilache edited the summary of this revision. (Show Details)
nicolasvasilache edited the summary of this revision. (Show Details)

Address review comments.

This revision was automatically updated to reflect the committed changes.