spirv-fuzz: Transformation to flatten conditional branch

[stefanomil]

This transformation, given the header of a selection construct with
branching instruction OpBranchConditional, flattens it.
Side-effecting operations such as OpLoad, OpStore and OpFunctionCall
are enclosed within smaller conditionals.
It is applicable if the construct does not contain inner selection
constructs or loops, or atomic or barrier instructions.

The corresponding fuzzer pass looks for selection headers and
tries to flatten them.

Needed for the issue #3544 , but it does not fix it completely.

[Vasniktel]

This is a very good PR. I left some review comments, although I didn't manage to review it completely.

[paulthomson]

This looks really good! I did not finish reviewing because I think we have some things to think about.

[Vasniktel]

This looks great. Code quality is amazing.

I left some review comments.

[afd]

This is a really great draft of a pretty difficult transformation!

I think it will take a couple of review rounds.

This review has a bunch of mainly stylistic comments.

There are two bigger changes I would like to see.

(1) For a problematic instruction you have a sequence of 3 or 5 fresh ids depending on the nature of the instruction. Instead I would like you to have a record with named fields, catering for the worst case number of ids that might be required, and populating fields that are not needed with zero. So, for instance, you could have:

• a field for the new 'execute' block
• a field for the 'alternative' block (you could use 0 if you don't need an alternative block, or you could just always have one)
• a field for the merge block
• a field for an OpPhi result (which can be zero if not needed)
• a field for the id to store a placeholder result in the alternative block (you could use zero if not needed).

If you give them all names that will make the transformation more readable.

(2) I am not totally set on this, but: you don't really need 'alternative' blocks. You could instead use a global value as the OpPhi argument, so that the selection header an jump to its merge or the 'execute' block, and the merge block can do an OpPhi between the result computed in the 'execute' block and a global value. In that case it would be good for the transformation to be provided with a (non-fresh) id for a suitable global value (which the fuzzer pass would create). Ideally this would be an irrelevant zero, and it should only be an undef in cases where a zero is not allowed.

If you would prefer to stick with 'alternative' blocks, then I would prefer you to favour creating an id that is a copy of an irrelevant zero over using OpUndef. I'm a little nervous about us introducing too much use of OpUndef, simply because the semantics of undefined values isn't that clear in SPIR-V.

[afd]

Thanks for making the changes! This is looking excellent. My probably-final feedback is to do with where the new comparator and nesting depth method should live, what you should call the new helper protobuf message, and whether you should have a helper class that mirrors the protobuf message.

Happy to discuss these things live if that would help, of course.

[stefanomil]

@afd There is a case that I previously had not considered: the first block of each branch could itself have OpPhi instruction, which needs to be updated in the second branch. Also, if the header branches to the same block (there is not an actual conditional), the OpPhi instructions in it do not need to be updated. Please look at the changes I made about this.

[afd]

Thanks! Some small things and then I'm happy to merge.

[afd]

Thanks for this contribution!