Page MenuHomePhabricator

[MLIR][SPIRV] Support identified and recursive structs.
ClosedPublic

Authored by ergawy on Sep 6 2020, 10:09 AM.

Details

Summary

This PR adds support for identified and recursive structs. This includes: parsing,
printing, serializing, and deserializing such structs.

The following C struct:

C
struct A {
  A* next;
};

which is translated to the following MLIR code as:

mlir
!spv.struct<A, (!spv.ptr<!spv.struct<A>, Generic>)>

would be represented in the SPIR-V module as:

spirv
OpName %A "A"
OpTypeForwardPointer %APtr Generic
%A = OpTypeStruct %APtr
%APtr = OpTypePointer Generic %A

In particular the following changes are included:

  • SPIR-V structs can now be either identified or literal (i.e. non-identified).
  • All structs now have their members surrounded by a ()-pair.
  • For recursive references, (1) an OpTypeForwardPointer instruction is emitted before the OpTypeStruct instruction defining the recursive struct (2) an OpTypePointer instruction is emitted after the OpTypeStruct instruction which actually defines the recursive pointer to struct type.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ergawy added inline comments.Sep 18 2020, 8:19 AM
mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp
115

+1, saves us a look up and don't see a downside to it. Done.

569

Just FYI, SetVector is indeed no re-exported.

1276

Initially, I refrained from doing so because of a small issue overlooked in the round-trip code of spv. In particular, the same context is used both for serialization and deserialization. However:

  • I adopted what you did in the LLVM dialect by failing only if the struct content is different from what is already set.
  • Started a review to handle the round-tripping issue: https://reviews.llvm.org/D87692.
ergawy updated this revision to Diff 292802.Sep 18 2020, 8:29 AM
ergawy marked 4 inline comments as done.
  • Remove some trivial braces.
ergawy updated this revision to Diff 292812.Sep 18 2020, 8:53 AM
  • Run clang-format.
ergawy edited the summary of this revision. (Show Details)Sep 18 2020, 8:58 AM
ergawy marked 6 inline comments as done.Sep 22 2020, 3:15 AM

Ping! :). I guess all comments from @ftynse are handled now. Please have another look whenever is suitable for you.

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

I will prepare a review with the proposed changes this week when I have some free time.

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

We do, the field and the argument have the same name.

ergawy updated this revision to Diff 294641.Sep 28 2020, 3:44 AM
ergawy marked 2 inline comments as done.
  • Rebase.
ftynse accepted this revision.Sep 29 2020, 5:26 AM

Type modeling LGTM. Sorry for the delay, this is a relatively complex and long patch that requires full attention. I would speculate that @rriddle picked at the same things as I did about type modeling. It would be great to have somebody more familiar with SPIR-V to take a look at the (de)serialization.

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

Nit: I believe the same comment as below about threads is applicable here. That is, if another thread creates (or indeed if any thread had already created a struct with the same identifier, that struct will be returned).

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

Nit: I'd also require the unused parts of the key to be always empty. This seems to be the case in practice and avoids bad surprises in the future.

946–950

(Out-of-scope for this commit): any reason why this is not using ArrayRef?

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

Nit: this isn't an ID anymore.

mlir/lib/Dialect/SPIRV/Transforms/DecorateSPIRVCompositeTypeLayoutPass.cpp
39

Ultra-nit: drop trivial braces

ergawy updated this revision to Diff 295201.Sep 30 2020, 1:35 AM
  • Handle review comments.
ergawy marked 4 inline comments as done.Sep 30 2020, 1:39 AM

@ftynse thanks for the approval, I appreciate you taking the time for a careful review (and apologies for always forgetting to remove trivial braces, in my personal coding I like to always add them :D. But if that's the style mostly followed in MLIR then I will follow it as closely as possible).

@antiagainst promised to have a look soon :P.

(and apologies for always forgetting to remove trivial braces, in my personal coding I like to always add them :D. But if that's the style mostly followed in MLIR then I will follow it as closely as possible).

No worries, I find switching back and forth between styles to be the hardest part :) Eliding trivial braces is a part of LLVM coding standard now - https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements - it has always been implicitly.

antiagainst accepted this revision.Sep 30 2020, 3:07 PM

This is impressive, thanks @ergawy for the great contribution! I love the extensive documentation and tests. :) Sorry for the delay; I have a little one to my family recently so that eats into my time and effort greatly. :) In general this looks good to me. Also Alex has been already given extensive reviews. I just have a few nits. I'll handle landing this after you addressed them. Thanks very much!

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

It would be nice to briefly talk about the two kinds of struct types: literal and identified in the documentation. Like:

Two kinds of struct types are supported: literal and identified. The former is uniqued via its fields while the later is uniqued via a string identifier. One typically see literal structs; identified structs are typically used for recursive structs.

322

Documentation for this? What if this struct is not identified?

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

This function is quite lengthy right now. We need to break it down. No action required for this change though. Could you put a TODO to remind?

600

It would be nice to document this regarding what it is used for and why thread_local.

602

Does this have to be static?

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

Nit: put two spaces to align info with an. :)

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

Nit: "expected ... type": use lower case and no trailing dot.

1457

Is this a tab?? :)

2468

TODO:

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

Just put this ahead of recursiveStructInfos in the class?

996

Document why this context for later reference?

1021

The else here is not necessary? and also a tab in the below. :) Make sure you run clang format.

ergawy updated this revision to Diff 295540.Oct 1 2020, 5:52 AM
  • Handle review comments.
ergawy marked 9 inline comments as done.Oct 1 2020, 5:59 AM

Sorry for the delay; I have a little one to my family recently so that eats into my time and effort greatly. :)

No worries and thanks for taking the time @antiagainst.

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

I declared it static since I didn't want a new lambda object to be created with each invocation.

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

Not sure why it shows a tab. Ran clang-format on the entire file.

I have concerns about several aspects of what you are doing here. Blocking until I have time to review.

Friendly ping to @rriddle. I'd like to land this and iterate on it if there are no major concerns from you.

ergawy updated this revision to Diff 296124.Oct 5 2020, 2:28 AM
  • Update tests to reflect latest master changes.
rriddle accepted this revision.Oct 5 2020, 1:20 PM

Apologies for any delays, this wasn't appearing in my dashboard. LGTM pending resolving comments.

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

Top-level comments should be ///.

326

nit: Can you add a comment here?

mlir/lib/Dialect/SPIRV/LayoutUtils.cpp
73

nit: Don't use else after return

73

nit: Drop else after return.

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

Is there an action item to remove this? We shouldn't be relying on thread_local ideally.

765

nit: Can you flip this conditional and early return the else branch instead?

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

nit: If you have an comment inside of the body, you need braces.

823

nit: Don't use else after return.

925

nit: Can you name the parameters something else to avoid the need for this->? Will avoid any potential problems refactoring this in the future.

948

Why the newline here?

950

nit: You could use a PointerIntPair to pack this boolean into one of the pointers to save space.

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

hmm, seems like the style in this file is inconsistent. Variable comments should use ///.

1238

nit: Drop the newline here.

1247

Why the use of std::begin/std::end? Also if the iterators aren't changing within the loop, you should cache the end iterator instead of recomputing it.

1250

Same here.

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

nit: Drop trivial braces.

1025

Is this something that we can test for?

This revision is now accepted and ready to land.Oct 5 2020, 1:20 PM
ergawy updated this revision to Diff 296433.Oct 6 2020, 6:17 AM
  • Rebase and handle review comments.
ergawy marked 15 inline comments as done.Oct 6 2020, 6:21 AM

Thanks for the review!

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

I don't whether the solution is good, but I started a review to get rid of this and also to provide a unified solution for managing parsing context in general: https://reviews.llvm.org/D88090.

If you think the solution is not good enough, then I will at least add a TODO to find a proper solution.

Out of curiosity, what are the downsides for using thread_local?

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

I fixed it for the newly added variables at least.

1247

Should I follow a different style when iterating over containers?

The containers are changed inside the loop so we need to recalculate the end iterator.

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

I think we can. I will follow up with a review to handle that TODO. Just thought this one is long as it is.

ergawy marked 3 inline comments as done.Oct 6 2020, 6:21 AM
ergawy added a comment.Oct 8 2020, 3:54 AM

Ping! Is it ok to merge this patch now? (I don't have commit access).

ergawy updated this revision to Diff 297525.Oct 12 2020, 2:55 AM
  • Rebase.
This revision was automatically updated to reflect the committed changes.