search

tensorflow / mlir

"Multi-Level Intermediate Representation" Compiler Infrastructure
1.74k stars 260 forks source link

[spirv] Extend spv.array with Layoutinfo #80

Closed denis0x0D closed 5 years ago

denis0x0D commented 5 years ago

This patch is addressing issue https://github.com/tensorflow/mlir/issues/76

  1. The ebnf is updated to support layoutinfo: For example, spv.array with ArrayStride information: !spv.array< 4 x !spv.array<4 x f32 [4]> [128]> The check for layoutinfo in the parser is relaxed, the checks in the (de)serializer generate erros in case ArrayStride is not present for OpTypeArray regarding to the spec.
  2. Tests related to serialization machinery updated.
  3. Changed specialization of template function from typename StructType::LayoutInfo_ to typename uint64_t to be able to reuse it for ArrayType::LayoutInfo.

@antiagainst @MaheshRavishankar can you please take a look? Thanks!

denis0x0D commented 5 years ago

@MaheshRavishankar thanks a lot for review! I've updated the patch, can you please take a look?

denis0x0D commented 5 years ago

@MaheshRavishankar thanks a lot for review!

denis0x0D commented 5 years ago

@antiagainst thanks a lot for review! I've fixed my incorrect understanding of specifications for ArrayStride, can you please take a look? Thanks.

denis0x0D commented 5 years ago

tests updated.

denis0x0D commented 5 years ago

@antiagainst @MaheshRavishankar if no one is interesting in the task https://github.com/tensorflow/mlir/issues/77 , can I take it? Thanks!

antiagainst commented 5 years ago

Sure! Please go ahead!

MaheshRavishankar commented 5 years ago

Thanks Denis for taking up the extra tasks. We have been busy with trying to resolve the sticky issue with https://github.com/tensorflow/mlir/issues/68.

denis0x0D commented 5 years ago

@antiagainst @MaheshRavishankar thanks, I'm happy to help with it. I've seen this issue https://github.com/tensorflow/mlir/issues/68 ,what the solution you've decided to take?

MaheshRavishankar commented 5 years ago

@denis0x0D I think we are going to have to change the way module level variables and constants are represented. They will have a symbol name associated with such variables/constants and wont have an SSA value. I am in the process of changing variables to be this way. Will post an update when it goes into github. There are a lot of advantages of not doing implicit captures in functions. But it doesnt match directly with SPIR-V spec, so some things have to change in the current dialect.

antiagainst commented 5 years ago

+1 to what @MaheshRavishankar said. We'll update #68 later to capture the offline discussion.

This pull request now has a conflict. Could you rebase against master, @denis0x0D? Thanks!

denis0x0D commented 5 years ago

@MaheshRavishankar thanks, sounds interesting! @antiagainst rebased on master. Thanks.

denis0x0D commented 5 years ago

BTW, it seems like CI build fails, because of hardcoded variable https://github.com/tensorflow/mlir/blob/master/CMakeLists.txt#L10 llvm was moved to C++14 recently and regarding to the https://gcc.gnu.org/projects/cxx-status.html#cxx14 gcc-5 must support c++14 standard.