Page MenuHomePhabricator

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

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



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

The following C struct:

struct A {
  A* next;

which is translated to the following MLIR code as:

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

would be represented in the SPIR-V module as:

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

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


Just FYI, SetVector is indeed no re-exported.


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:
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.


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


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.


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).


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.


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


Nit: this isn't an ID anymore.


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 - - 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!


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.


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


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?


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


Does this have to be static?


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


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


Is this a tab?? :)




Just put this ahead of recursiveStructInfos in the class?


Document why this context for later reference?


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.


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


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.


Top-level comments should be ///.


nit: Can you add a comment here?


nit: Don't use else after return


nit: Drop else after return.


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


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


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


nit: Don't use else after return.


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


Why the newline here?


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


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


nit: Drop the newline here.


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.


Same here.


nit: Drop trivial braces.


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!


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:

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?


I fixed it for the newly added variables at least.


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.


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.