[SPIR-V 1.6] True Label and False Label of an OpBranchConditional must not be the same #2528
Conversation
…t not be the same
| @@ -859,6 +859,8 @@ class SPIRVBranchConditional : public SPIRVInstruction { | |||
| getCondition()->getType()->isTypeBool()); | |||
| assert(getTrueLabel()->isForward() || getTrueLabel()->isLabel()); | |||
| assert(getFalseLabel()->isForward() || getFalseLabel()->isLabel()); | |||
| if (Module->isAllowedToUseVersion(VersionNumber::SPIRV_1_6)) | |||
There was a problem hiding this comment.
I'm not filling strong about adding this assert as it's not an assertion of our implementation, is there a way to add an error message? We have probably discussed it during another PR, so sorry for bothering, but still could you please remind me the result?
There was a problem hiding this comment.
@MrSidims Actually we didn't decide anything about error mechanism: #2482 (comment)
I'm not sure again if it's related to the PR, but it seems we should rework all the validate() methods to get rid of assertions. Will this be one day replaced with the validator from the SPIRV-Tools?
There was a problem hiding this comment.
Isn't this too dangerous? What if someone had input LLVM that previously worked, but it starts failing with this assert?
There was a problem hiding this comment.
@LU-JOHN would you mind to provide an example of such module?
There was a problem hiding this comment.
This code is silly, but valid:
define spir_func void @func_export(i16 %a) {
entry:
%cmp = icmp eq i16 %a, 0
br i1 %cmp, label %same, label %samesame:
ret void
}
There was a problem hiding this comment.
I wonder why this restriction was added. What positive impact does it have?
On a more relevant note, I think this PR might need a test. Thanks
There was a problem hiding this comment.
This code is silly, but valid:
And how can we generate such module to break existing flows? But may be we indeed shall not add an assertion, instead we should just skip such branch instructions during translation, but I'd like to see C examples. I've attempted to get one with goto, but failed. We can't get such BB as a loop's latch BB, right?
What positive impact does it have?
If I'm not mistaken, the idea is to ensure maximum convergence in generated SPIR-V modules, for this you need to have as minimum re-convergence points as possible (there is more work for this, but it's not applicable to this very change). In the example provided by John the branch instruction is useless and can be removed supporting minimum re-convergence goal.
There was a problem hiding this comment.
I couldn't make an example from C either.
No description provided.