Page MenuHomePhabricator

[MLIR][SPIRVToLLVM] SPIR-V types size in bytes function
ClosedPublic

Authored by georgemitenkov on Jul 7 2020, 1:53 AM.

Details

Summary

Added getTypeNumBytes() function as a class member to several SPIR-V Types:

  • Scalar
  • Array
  • Vector
  • Pointer

This is a function that will expose getTypeNumBytes() from SPIRVLowering.cpp to other classes. Support of other SPIR-V types can be added on demand.

Diff Detail

Event Timeline

georgemitenkov created this revision.Jul 7 2020, 1:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2020, 1:53 AM

@antiagainst, @mravishankar I haven't linked getTypeNumBytes() from SPIRVLowering.cpp to this new implementation? Should this be done as well?

antiagainst requested changes to this revision.Jul 7 2020, 6:47 PM
antiagainst added inline comments.
mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
113

Nit: The Type in the method does not help much given this is a method for .. type. :)

Typically think the method name and variable name and other name as the first line of documentation. A good name can go a long way for helping others understanding the code without referencing to the documentation and others additional stuff. For example, NumBytes is good here given it's very clear the unit of the return number so one does not need to guess whether is in bytes or bits or whatever. What do you think about getSizeInBytes here?

Also we need to document it here. What if the type can have explicit layout? What if not? What does it mean to return None?

mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp
159

We need to have documentation on the ArrayType mentioning the padding behavior here.

300

Is this correct? Don't we need to multiply with the count?

640

Hmm, this is not correct actually. There is no defined size for SPIR-V pointer type... We should return None here actually. I think even for LLVM, the pointer type size isn't fixed; it depends on the data layout.

906

No need to have these elses given we return for each case.

This revision now requires changes to proceed.Jul 7 2020, 6:47 PM
georgemitenkov marked 5 inline comments as done.

Renamed getTypeNumBytes() to getSizeInBytes().
Added documentation for the function declaration and a comment on array strides.
Removed support of PointerType and fixed Vector size.

antiagainst accepted this revision.Jul 9 2020, 5:59 AM
antiagainst added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp
159

Sorry I meant this should be on the declaration side (.h); not the implementation side (.cpp). :)

This revision is now accepted and ready to land.Jul 9 2020, 5:59 AM
georgemitenkov marked an inline comment as done.Jul 9 2020, 7:05 AM

Moved stride comment on SPIRV's array type getSizeInBytes() from .cpp to .h file.

This revision was automatically updated to reflect the committed changes.