Page MenuHomePhabricator

[MLIR] Introduce memref vector cast op
Needs RevisionPublic

Authored by bondhugula on Aug 13 2020, 2:40 AM.

Details

Summary

Introduce memref_vector_cast op, which casts a memref of an elemental type
to a memref of a vector of that elemental type. This only supports
1-d vectorization of the shape and is meant to be used for vectorization
in the common case. Lowering to LLVM included.

Diff Detail

Unit TestsFailed

TimeTest
60 mslinux > flang-Unit.Lower/_/FlangLoweringOpenMPTests::OpenMPLoweringTest.Barrier
Note: Google Test filter = OpenMPLoweringTest.Barrier [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
80 mslinux > flang-Unit.Lower/_/FlangLoweringOpenMPTests::OpenMPLoweringTest.EmptyParallel
Note: Google Test filter = OpenMPLoweringTest.EmptyParallel [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
50 mslinux > flang-Unit.Lower/_/FlangLoweringOpenMPTests::OpenMPLoweringTest.TaskWait
Note: Google Test filter = OpenMPLoweringTest.TaskWait [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
50 mslinux > flang-Unit.Lower/_/FlangLoweringOpenMPTests::OpenMPLoweringTest.TaskYield
Note: Google Test filter = OpenMPLoweringTest.TaskYield [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.

Event Timeline

bondhugula created this revision.Aug 13 2020, 2:40 AM
Herald added a project: Restricted Project. · View Herald Transcript
bondhugula requested review of this revision.Aug 13 2020, 2:40 AM

Combine match and rewrite; update LLVM lowering style.

Fix inadvertent change.

Fix yet another inadvertent change.

Harbormaster completed remote builds in B68233: Diff 285296.
ftynse requested changes to this revision.Aug 13 2020, 5:52 AM

Was there an RFC for this op? How this connects to https://llvm.discourse.group/t/memref-cast/1514?

Can this be used as a replacement/generalization of https://mlir.llvm.org/docs/Dialects/Vector/#vectortype_cast-vectortypecastop ?

This revision now requires changes to proceed.Aug 13 2020, 5:52 AM
ftynse added inline comments.Aug 13 2020, 6:02 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2354

Type conversion may fail, please check the result for null and bail out on errors.

2356

MemRefDescriptor::undef(targetStructType)

2365

Please use MemRefDescriptor API to work with descriptors instead of spreading around the assumptions about the descriptor structure.

2381

I can't seem to find a verifier check that prevents this, so the assert may trigger on valid IR.

2384

This would have been one line using MemRefDescriptor API

bondhugula added a comment.EditedAug 13 2020, 7:17 AM

Was there an RFC for this op? How this connects to https://llvm.discourse.group/t/memref-cast/1514?

See later in the discussion you are referring to:
https://llvm.discourse.group/t/memref-cast/1514/10

There were multiple requests for having this op contributed - this can be in the future integrated into one of the memref cast ops.
https://llvm.discourse.group/t/understanding-the-vector-abstraction-in-mlir/989/16
https://llvm.discourse.group/t/memref-cast/1514/10

Can this be used as a replacement/generalization of https://mlir.llvm.org/docs/Dialects/Vector/#vectortype_cast-vectortypecastop ?

Yes, this op can subsume such a cast. To start with, I don't think that vector type cast should have ideally been introduced in the vector dialect; my guess is that it was temporary and for local experimentation before it could be subsumed or generalized by something in the standard dialect. Because that's really a cast on memref's - not vectors.

In any case, I expect this memref_vector_cast to potentially be merged into another cast (memref_reinterpret_cast for example) depending on how loaded we want each of the cast ops to be.

bondhugula marked 2 inline comments as done.Aug 18 2020, 5:31 PM
bondhugula added inline comments.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2381

Changed to return failure - thanks!

2384

Thanks - switched to the memref descriptor API.

bondhugula marked 2 inline comments as done.Aug 18 2020, 5:31 PM

Address review comments.

bondhugula marked 3 inline comments as done.

Address remaining review comments.

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2356

Thanks.

aartbik added inline comments.Aug 18 2020, 8:08 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1894

or rank (also not changed)

1895

unless that last dimension is a ?

I would add some more explanation on what is allowed in those cases

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2342

this is a pretty big block of memref related manipulations without any comments
please add some for readability of what is being done where

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1980

don't you want to test that the division is exact, i.e. no remainder?

bondhugula marked 2 inline comments as done.Aug 18 2020, 11:18 PM
bondhugula added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1894

Thanks, updated. It's actually more than that: 'rank' as well as the extents along all but the last dimension remain unchanged the way it is.

1895

Even if the last dimension is dynamic, it's still supported. We divide the dynamic size by the vector width. So it works for both static and dynamic and the lowering already supports it. The second example below has it.

bondhugula marked 2 inline comments as done.

Update doc comment

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

If it wants to subsume vector.type_cast, how does this interact with n-D vectors for n>1?

There are also implications related to data layout that I don't see how to handle in the general case.
This is the main reason vector.type_cast exists: it caters to a very specific use case.

This revision conveniently omits any discussion about layout which is critical.
How does this all work with non-"powers of 2" vectors?

This revision now requires changes to proceed.Aug 19 2020, 1:27 AM

This functionality makes it very straightforward to develop SIMD code and replicate at a high level the ability to view memory as either scalar or SIMD code with very few restrictions.

ftynse resigned from this revision.Aug 19 2020, 7:40 AM

My implementation-level concerns have been addressed. Leaving the floor to others for conceptual discussions.

bondhugula added a comment.EditedAug 19 2020, 10:25 AM

If it wants to subsume vector.type_cast, how does this interact with n-D vectors for n>1?

More than 1-d has not really been thought out. I guess I just meant it could subsume what vector type_cast would do 1-d vectors.

There are also implications related to data layout that I don't see how to handle in the general case.

Yes, the implications with layout come in for the case of n > 1. This isn't really meant / designed for that.

This is the main reason vector.type_cast exists: it caters to a very specific use case.

This revision conveniently omits any discussion about layout which is critical.

Are you referring to non-identity layouts for memrefs? This isn't really meant to handle non-identity layouts. I'm not sure what you mean by "discussion" - this isn't a paper or a document to omit or include discussion. This is just implementing the functionality it describes that's really missing and that multiple folks requested for and need. I also see the adjective "conveniently omits" as making absolutely no sense in this context.

How does this all work with non-"powers of 2" vectors?

There is no restriction on the vector width as far as I can see. It's just a floordiv by vector width. Separately and unrelatedly, I'm yet to see hardware with non power of two vectors though. Why should something deal with non power of two vectors to be useful?

aartbik added inline comments.Aug 19 2020, 2:23 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1895

Yes, I realized that. My comment was indented to ask for proper documentation of that case, since the text and example did not fully agree (it is unclear if ? is at least vector sized for example while reading the text). Sorry if that was not clear.

bondhugula marked an inline comment as done.Aug 26 2020, 9:23 AM
bondhugula added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1895

That's right, whether ? or constant, the last dimension is expected to be vector sized. Let me know if you want me to augment this doc in some other way.