Page MenuHomePhabricator

[mlir][spirv] modify structure type member decoration and layout information handling
ClosedPublic

Authored by Hazem on Jun 8 2020, 1:23 PM.

Details

Summary

Modify structure type in SPIRV dialect to support:

  1. Multiple decorations per structure member
  2. Key-value based decorations (e.g., MatrixStride)

This commit kept the Offset decoration separate from members' decorations container for easier implementation and logical clarity.
As such, all references to Structure layoutinfo are now offsetinfo, and any member layout defining decoration (e.g., RowMajor for Matrix)
will be add to the members' decorations container along with its value if any.

Diff Detail

Event Timeline

Hazem created this revision.Jun 8 2020, 1:23 PM
Herald added a project: Restricted Project. · View Herald Transcript
rriddle added inline comments.Jun 8 2020, 1:27 PM
mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
306

I would have expected this to be next to memberIndex if you wanted to pack it.

307

nit: Remove the inline in class/struct definitions, it goes against LLVM style guide

https://llvm.org/docs/CodingStandards.html#don-t-use-inline-when-defining-a-function-in-a-class-definition

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

nit: Merge these lines.

mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp
1334

nit: Use braces on all or none.

Hazem updated this revision to Diff 269338.Jun 8 2020, 1:29 PM

Updating D81426: [mlir][spirv] modify structure type member decoration and layout information handling

Minor access specifier modification.

Hazem marked 4 inline comments as done.Jun 8 2020, 1:35 PM

Thanks for the patch, added some stylistic nits.

Hazem updated this revision to Diff 269360.Jun 8 2020, 2:31 PM
  • address few nits after revision review
antiagainst requested changes to this revision.Jun 9 2020, 5:41 PM

Nice!!! Thanks @Hazem for the contribution!

mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
278–299

Could you put some doc for these two types?

// Type for specifying the offset of struct members

// Type for specifying the decoration(s) on struct members
306

+1. I think River meant

uint32_t memberIndex : 31;
uint32_t hasValue : 1;
364

Prefer meaningful variable names especially in APIs. The name itself can be part of the documentation. f and other one-char names should really have a very small scope inside implementation.

mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
571–587

Nit: end with . to be consistent with other comments.

579

We don't need the explicit spirv::StructType::MemberDecorationInfo with emplace_back? Or is a constructor missing for MemberDecorationInfo?

mlir/lib/Dialect/SPIRV/Serialization/Serializer.cpp
232

Use const & to avoid copying?

This revision now requires changes to proceed.Jun 9 2020, 5:41 PM
Hazem marked 6 inline comments as done.Jun 10 2020, 8:30 AM
Hazem added inline comments.
mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
306

I understand, but when I did that I noticed unexpected behavior from the code. I believe I might need to cast the memberIndex to uint32_t explicitly in certain usages. So I resorted to the easier solution and forsake the few bytes savings. I will give it another shot and see what is wrong with it.

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

There is no constructor for the MemberDecorationInfo struct.

Hazem updated this revision to Diff 269867.Jun 10 2020, 8:50 AM
  • Addressing more revision comments
Hazem marked an inline comment as done.Jun 10 2020, 10:17 AM

@antigainst, I made all the changes but I am failing the build I believe because of llvm::hash_value method in clang-tidy, do you know how can I workaround this? the function has to be defined this way.

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

All set now with the recommended changes for packing struct fields.

antiagainst accepted this revision.Jun 10 2020, 4:28 PM

Thanks again @Hazem for the contribution!

@antigainst, I made all the changes but I am failing the build I believe because of llvm::hash_value method in clang-tidy, do you know how can I workaround this? the function has to be defined this way.

Don't worry about that. It's not useful for this case.

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

Sorry for the additional trouble there. Compilers want to generate fast code and use the least resource; but that applies to the compiler itself. ;-P

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

I meant we can define a constructor for it so that we can then avoid the extra type wrapper here. Let me handle it before landing.

This revision is now accepted and ready to land.Jun 10 2020, 4:28 PM
This revision was automatically updated to reflect the committed changes.
rriddle marked an inline comment as done.Jun 10 2020, 4:44 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
307

Methods defined in the class definition are implicitly inline.

mehdi_amini added inline comments.Jun 10 2020, 5:28 PM
mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
307

Actually explicitly adding inline still hints the inliner to be more aggressive I believe.
(but unless you have a benchmark, just don't)

I believe this change may have broken the Windows buildbot, here: http://lab.llvm.org:8011/builders/mlir-windows/builds/3068

933.041 [440/64/2382] Building CXX object tools\mlir\lib\Dialect\SPIRV\CMakeFiles\obj.MLIRSPIRV.dir\TargetAndABI.cpp.obj
FAILED: tools/mlir/lib/Dialect/SPIRV/CMakeFiles/obj.MLIRSPIRV.dir/TargetAndABI.cpp.obj 
C:\PROGRA~2\MICROS~1\2017\COMMUN~1\VC\Tools\MSVC\1416~1.270\bin\Hostx64\x64\cl.exe ... /showIncludes /Fotools\mlir\lib\Dialect\SPIRV\CMakeFiles\obj.MLIRSPIRV.dir\TargetAndABI.cpp.obj /Fdtools\mlir\lib\Dialect\SPIRV\CMakeFiles\obj.MLIRSPIRV.dir\ /FS -c E:\build_slave\mlir-x64-windows-ninja\llvm-project\mlir\lib\Dialect\SPIRV\TargetAndABI.cpp
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\utility(319): error C2079: 'std::pair<mlir::Identifier,mlir::Attribute>::first' uses undefined class 'mlir::Identifier'
E:\build_slave\mlir-x64-windows-ninja\llvm-project\mlir\include\mlir/Dialect/SPIRV/SPIRVTypes.h(303): note: see reference to class template instantiation 'std::pair<mlir::Identifier,mlir::Attribute>' being compiled
rriddle added inline comments.Jun 10 2020, 6:00 PM
mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
307

Yes, explicitly providing the keyword(at least for clang) tags functions with inline hint. The performance impact of explicitly providing the keyword in the majority(especially these) of cases is negligible/nonexistent.

mehdi_amini added a comment.EditedJun 10 2020, 7:24 PM

I believe this change may have broken the Windows buildbot, here: http://lab.llvm.org:8011/builders/mlir-windows/builds/3068

I reverted earlier (it also broke the GCC build)

I checked and the windows bot is green now.

Thanks for the test failure report! I've pushed https://github.com/llvm/llvm-project/commit/5d74df5b03e46b7bd3700e3595c7008a6905b288 with the fix. I've verified that it works on GCC 8 using the configuration in https://buildkite.com/mlir/mlir-core/builds/5556#47fec22d-d9e2-4784-b660-34fe82d6dd0b. The Windows failure and GCC failure look the same.

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

I think I misplaced my previous comment. It was meant for the bit packing in MemberDecorationInfo.

The Windows failure and GCC failure look the same.

Unfortunately it looks like this is causing a new failure since <bits/stdint-uintn.h> isn't available on Windows. I'm not sure I understand what types it provides that wouldn't otherwise be available, but is there any alternative source? Thanks again for looking into this!
http://lab.llvm.org:8011/builders/mlir-windows/builds/3125

The Windows failure and GCC failure look the same.

Unfortunately it looks like this is causing a new failure since <bits/stdint-uintn.h> isn't available on Windows. I'm not sure I understand what types it provides that wouldn't otherwise be available, but is there any alternative source? Thanks again for looking into this!
http://lab.llvm.org:8011/builders/mlir-windows/builds/3125

Reverted in 6f0ce46873b609851634b2c77fc76bf8d580c3c6

Hazem added a comment.Jun 12 2020, 9:43 AM

I am not sure I am following up, can someone explain to me why we needed to revert this change and then add "bits/stdint-uintn.h" in the first place? The link (http://lab.llvm.org:8011/builders/mlir-windows/builds/3068) to the initial change that caused the original issue wasn't related to this particular patch. Or maybe I misunderstood the messages.

The Windows failure and GCC failure look the same.

Unfortunately it looks like this is causing a new failure since <bits/stdint-uintn.h> isn't available on Windows. I'm not sure I understand what types it provides that wouldn't otherwise be available, but is there any alternative source? Thanks again for looking into this!
http://lab.llvm.org:8011/builders/mlir-windows/builds/3125

Reverted in 6f0ce46873b609851634b2c77fc76bf8d580c3c6

antiagainst added a comment.EditedJun 12 2020, 2:51 PM

The Windows failure and GCC failure look the same.

Unfortunately it looks like this is causing a new failure since <bits/stdint-uintn.h> isn't available on Windows. I'm not sure I understand what types it provides that wouldn't otherwise be available, but is there any alternative source? Thanks again for looking into this!
http://lab.llvm.org:8011/builders/mlir-windows/builds/3125

Reverted in 6f0ce46873b609851634b2c77fc76bf8d580c3c6

Okay, it's clangd trying to be smart and automatically adding headers for me... This type it picked <bits/stdint-uintn.h> when I changed this->decoration < other.decoration to static_cast<uint32_t>(this->decoration) < static_cast<uint32_t>(other.decoration). Sorry about that.

I am not sure I am following up, can someone explain to me why we needed to revert this change and then add "bits/stdint-uintn.h" in the first place? The link (http://lab.llvm.org:8011/builders/mlir-windows/builds/3068) to the initial change that caused the original issue wasn't related to this particular patch. Or maybe I misunderstood the messages.

GCC and MSVC is not happy with this->decoration < other.decoration. Enum classes will not implicitly convert to integers. I changed to static_cast<uint32_t>(this->decoration) < static_cast<uint32_t>(other.decoration) to fix.

I am not sure I am following up, can someone explain to me why we needed to revert this change and then add "bits/stdint-uintn.h" in the first place? The link (http://lab.llvm.org:8011/builders/mlir-windows/builds/3068) to the initial change that caused the original issue wasn't related to this particular patch. Or maybe I misunderstood the messages.

Since the build failed right after it was committed, and reverted it fixed the build, that seems like a good indication that it was related :)

Great! Thanks for figuring this one out.

Hazem added a comment.Jun 15 2020, 8:56 AM

I am not sure I am following up, can someone explain to me why we needed to revert this change and then add "bits/stdint-uintn.h" in the first place? The link (http://lab.llvm.org:8011/builders/mlir-windows/builds/3068) to the initial change that caused the original issue wasn't related to this particular patch. Or maybe I misunderstood the messages.

GCC and MSVC is not happy with this->decoration < other.decoration. Enum classes will not implicitly convert to integers. I changed to static_cast<uint32_t>(this->decoration) < static_cast<uint32_t>(other.decoration) to fix.

Thank you for the explanation. I missed casting this when I changed them back to be bit fields from uint32_t.

Hazem added a comment.Jun 15 2020, 9:01 AM

I am not sure I am following up, can someone explain to me why we needed to revert this change and then add "bits/stdint-uintn.h" in the first place? The link (http://lab.llvm.org:8011/builders/mlir-windows/builds/3068) to the initial change that caused the original issue wasn't related to this particular patch. Or maybe I misunderstood the messages.

Since the build failed right after it was committed, and reverted it fixed the build, that seems like a good indication that it was related :)

This part I understand from your previous comments. However, I am new to this contribution system so I was hoping to figure out what is wrong from the log messages so that I can fix it as well whenever it fails. So my question was why the particular changes I made fail the build and whether there are other log files/reports I am missing here to help me investigate it as the indicated log was referring to a totaly different change :)
But all is good now I will figure it out in the upcoming contributions as I go. Thanks!