Don't inline function containing OpKill

[s-perron]

If an OpKill instruction is inlined into a continue construct, then the
spir-v is no longer valid. To avoid this issue, we do not inline functions containing an
OpKill at all. This method was chosen because it is difficult to keep
track of whether or not you are in a continue construct while changing
the function that is being inlined into. This will work well with wrap
OpKill because all other instructions will still be inlined.

Fixes #2454
Fixes #2433

[greg-lunarg]

To avoid this issue, we do inline into an OpKill at all.

I am having trouble parsing this sentence.

This will work well with wrap OpKill because every will still be inlined except for the OpKill instruction itself.

I am having trouble parsing this sentence too.

[s-perron]

Sorry, I should proof read better. I've updated it.

[greg-lunarg]

I am sorry I didn't jump in on the triggering issue sooner.

My biggest concern with this solution is that we are going to require folks who roll their own optimization to know about this restriction, a restriction which to me seems fairly unintuitive. It will also require them to invoke a separate function wrapping pass to get the optimization that they would normally expect from an inliner.

It seems strange to me that a valid program can be made invalid through inlining, and that an invalid programming can be made valid by wrapping an instruction in a function. Judging by the threads coming from the triggering issue, I think everyone involved shares this concern to a certain extent.

The current situation suggests to me that our definition of what a valid program is a little out of whack. As such, it seems to me that it might be better to fix the problem by changing the definition of a valid program rather than changing the optimizer.

Given that, with this solution, we are allowing OpKill to be executed within a continue construct, albeit wrapped in its own function, it seems that outlawing OpKill within the call tree of a continue construct is not the way to go.

I would rather suggest the following: where the spec says the merge block must post-dominate the continue target, we add "ignoring OpKill". That is, for the purposes of continue target validation, an OpKill in a continue construct is treated as, for example, an unconditional branch to the merge block.

Given this, we would no longer need people to be aware that we don't inline functions with OpKill, which is somewhat unintuitive, and require them to preventatively call a separate pass to wrap OpKill in a function.

If we think this all makes sense, for consistency we will probably want to actually add "ignoring OpKill and OpReturn", since OpReturn has a similar effect on a continue construct, though it lacks the interaction with inlining and function wrapping that OpKill has.

Thoughts?

[s-perron]

I already brought this up with the spir-v committee. See the gitlab issue https://gitlab.khronos.org/spirv/SPIR-V/issues/459.

[greg-lunarg]

Thanks for the link. It is very helpful to know what has already been discussed.

FWIW, my proposal was not one of the alternatives that were considered, so I still feel it might be worth bringing up. While I agree allowing OpKill to appear in a continue construct is not a highly used capability, it would allow us to easily remove the unintuitive and draconian restriction that functions with OpKill cannot be inlined, so that is something.

Still, there may be another alternative which is not quite as good but would probably be acceptable in terms of code quality, would let us remove this restriction and wouldn't require us to go back through the SPIR-V WG:

Have the inliner do the OpKill wrapping itself. It would do the wrapping pass as the first thing before any inlining. A slight variation would be to have the inliner do the wrapping as part of the inlining process.

The advantage here is that the user does not have to know anything about OpKill, inlining, and wrapping.

Thoughts?

[s-perron]

Yes that could work. In general, I prefer keeping each pass as simple as possible. That is why I separated them. So I don't know how you were thinking of implementing this, but if we were to do something like that, I would want the options and api calls to imply both passes instead of having a single pass.

If you have concerns about this, open another issue to discuss it.

[greg-lunarg]

How does this PR fix #2554 ?

[s-perron]

Typo. Updated to #2454.