This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add support for converting memref of vector to SPIR-V
ClosedPublic

Authored by ThomasRaoux on Jul 30 2020, 1:56 PM.

Details

Summary

This allow declaring buffers and alloc of vectors so that we can support vector load/store.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Jul 30 2020, 1:56 PM
ThomasRaoux requested review of this revision.Jul 30 2020, 1:56 PM
mravishankar requested changes to this revision.Jul 30 2020, 2:22 PM
mravishankar added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
353–363

Maybe we can have something like

Type elementType = type.getElementType();
Type arrayElemType = nullptr;
if (auto vecType = dyn_cast<VectorType>(elementType)) {
   arrayElementType = ...
} else if (auto scalarType = ...) {
   arrayElementType = ...
} else {
   return llvm::None
}

?

357

double negative is confusing. Maybe say

"illegal: can convert memrefs of either scalar or vector element type only"

Though I dont know why these are "illegal". It should be "unhandled"

This revision now requires changes to proceed.Jul 30 2020, 2:22 PM

Address review comments

ThomasRaoux marked 2 inline comments as done.Jul 30 2020, 2:35 PM
ThomasRaoux added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
357

Sure, I rephrased it.

mravishankar accepted this revision.Jul 30 2020, 2:47 PM
mravishankar added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
353

I dont think this needs to be Optional<...> . You can initialize it to nullptr.

357

Nit: Maybe just compute the type.getElementType() once before the if? Would be easier to read and same number of lines.

This revision is now accepted and ready to land.Jul 30 2020, 2:47 PM
ThomasRaoux marked an inline comment as done.
ThomasRaoux marked an inline comment as done.
ThomasRaoux added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
353

convertScalarType and convertVectorType return Optional<Type> so I would need extra handling if I don't use Optional<>. I think that's the most natural way to do it? What do you think?

mravishankar added inline comments.Jul 30 2020, 3:04 PM
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
353

I didnt realize that. Then this is fine. Sorry for the noise.

mravishankar accepted this revision.Jul 30 2020, 3:04 PM