This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Improve stride support in array types
ClosedPublic

Authored by antiagainst on Apr 13 2020, 9:50 AM.

Details

Summary

This commit added stride support in runtime array types. It also
adjusted the assembly form for the stride from [N] to stride=N.
This makes the IR more readable, especially for the cases where
one mix array types and struct types.

Diff Detail

Event Timeline

antiagainst created this revision.Apr 13 2020, 9:50 AM
denis13 accepted this revision.Apr 13 2020, 10:58 AM

LGTM! Looks more pretty than it was, thanks!

mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
548–549

What do you think about

if (!type.getArrayStride())
555

What do you think about

if (!type.getArrayStride())
This revision is now accepted and ready to land.Apr 13 2020, 10:58 AM
denis13 added inline comments.Apr 13 2020, 11:00 AM
mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
548–549

Sorry, I meant

if (type.getArrayStride())
555

Sorry I meant

if (type.getArrayStide())
antiagainst marked 5 inline comments as done.Apr 13 2020, 11:13 AM
antiagainst added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
555

Oops, good catch! I meant to reuse the stride value in the following line. Fixed. :)

This revision was automatically updated to reflect the committed changes.
antiagainst marked an inline comment as done.
mravishankar reopened this revision.Apr 13 2020, 12:45 PM

Sorry, just getting to this for review.

See comments below.

mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
157

Seems to be a mismatch here. elementCount and stride should be size_t. Of course on many devices unsigned is enough, but when possible we might need to support objects of large sizes. Maybe better to make both size_t?

mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
231

This makes it too verbose. Why change the parsing/printing?

This revision is now accepted and ready to land.Apr 13 2020, 12:45 PM
mravishankar requested changes to this revision.Apr 13 2020, 12:47 PM

I know this is already committed, but I just wanted to reopen this (posted comments here). Didnt get around to reviewing it till now

This revision now requires changes to proceed.Apr 13 2020, 12:47 PM
antiagainst marked 4 inline comments as done.Apr 13 2020, 1:22 PM
antiagainst added inline comments.
mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
157

SGTM to be future proof. I used unsigned here to be consistent; better to change them together. (The serialization/deserialization side we need to be careful; the array stride is directly embedded as a literal there. I wonder whether all the drivers support non-32-bit cases nicely though. ;-P)

mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
231

It makes the IR more readable. Otherwise if you have structs containing arrays, everything (struct member offsets, array strides) are represented as [N]. It's really hard to differentiate. And I typically think [N] is more naturally for indexing; thus good for offsets.

mravishankar accepted this revision.Apr 16 2020, 10:02 AM
This revision is now accepted and ready to land.Apr 16 2020, 10:02 AM
bondhugula added inline comments.
mlir/test/Dialect/SPIRV/structure-ops.mlir
61

You don't need the %<num> = part. In any case, please don't use numbers in such places.

antiagainst closed this revision.Apr 20 2020, 8:54 AM
antiagainst marked 4 inline comments as done.
antiagainst added inline comments.
mlir/test/Dialect/SPIRV/structure-ops.mlir
61