Move OpPhi into BasicBlock

[kvark]

The spec says that OpPhi has to appear within a block before any other instruction (except for "OpLabel", which is already hard-wired to the BasicBlock). Would it make sense to represent it as this?

/// Data representation of a SPIR-V basic block.
#[derive(Debug, Default)]
pub struct BasicBlock {
    /// The label starting this basic block.
    pub label: Option<Instruction>,
    /// Variable control flow control for this basic block.
    pub phi: Option<Instruction>,
    /// Instructions in this basic block.
    pub instructions: Vec<Instruction>,
}

[kvark]

This would allow us to have a class on this Op (say "FunctionStruct"), which would in turn make the structured implementation more straightforward.

[kvark]

@antiagainst please see the draft implementation in 6541019

[Skepfyr]

If we were to do this then it would probably make sense to also make the label non-optional and have another field that stores the termination instruction that must be the last instruction in the block (and maybe even an optional merge that must be the second to last). These define a block so the only way you could end up not being able to make a block is if a function isn't followed by an OpLabel (ie. you know a block has started but you have no label) or if you reach the end of a function or the end of the stream without seeing a termination instruction.
I feel like this validation would almost certainly be useful for consumers but definitely increases the amount of validation the parser does (although it's probably best placed to do it).

[kvark]

The data representation is currently written (consistently!) in a way that makes most of the things optional (that are otherwise not by the spec). Addressing this, if requested, should be a separate effort/issue.

Saying that, the WIP structured representation (see #46) doesn't do it and instead makes required things be actually non-optional, so fixing the data representation isn't a huge priority to me at this point.

[antiagainst]

I initially designed the DR as-is to also make it possible to have minimal verification in the parser itself and support invalid cases so that we can even import broken SPIR-V blobs and investigate what is wrong with it. Now I feel whether that's useful is up to debate. I'm not opposed to revisiting this if it become a burden to maintain. If to drop this guarantee, then mandating the leading OpLabel and terminator instruction SGTM; but we probably can defer this as said by @kvark.

But I don't quite see why we'd like to special case OpPhi here? What problem are we trying to solve? Why leaving it in the instructions vector not acceptable?

[kvark]

I don't mind DR going either way.
For SR, I think it would make a lot of sense to have OpPhi as a separate struct that is enforced to be at the beginning of a block. The way I'm doing it now, decision to treat an instruction as a separate struct is taken based on the class only. Currently, OpPhi doesn't have a class assigned, so we have the following choices:

1. don't do anything with OpPhi, let it be an instruction in the list
2. hard-code SR codegen to detect OpPhi by something other than the class and put it into a separate struct
3. assign a class to OpPhi and fix the DR to handle it properly (without changing the way OpPhi is represented)
4. fully separate OpPhi - what Special OpPhi handling as a prelude to BasicBlock #59 does

I would prefer (3) or (4). @antiagainst what choice do you suggest?

[Skepfyr]

Shouldn't SR have its own BasicBlock type that can store all the extra information? I feel like that would solve the problem, although I may have misunderstood how the DR and SR are seperated.

[kvark]

Yes, SR will have it's own block struct. It doesn't require DR to respect OpPhi in the same way, only choice (4) does that.

[antiagainst]

Going more structured in SR makes sense for me (well that's the purpose)! So SGTM to have separate struct and place it at the beginning of BasicBlock SR.

I'm still not sold on changing DR to specially treat phi though. It's strange for us to only specially handle phi but not other terminators etc; it feels in an inconsistent state. If we want to do it, better to also consider having the terminator separated out from the list of instructions.

(Another annoying thing that I forgot to mention is OpLine. DR doesn't handle OpLine properly. IMO OpLine is a strange thing in the spec given it can appear almost everywhere, even at structural places, which breaks the module structure... Making the DR more "structured' we will also encounter this more too.)

What do you mean by "fix the DR to handle it properly (without changing the way OpPhi is represented)" in 3? If it means still keeping OpPhi as an instruction in the main Vec<Instruction> filed, then that would be my preference.

But if you'd really like to separate phi reprenstation out in DR, please also do it for terminators.

[kvark]

I'll just proceed with only recognizing OpPhi at the SR level. Thank you for your thoughts!