Fix LoopMerge Instruction Placement #3277
Conversation
|
Restarting CI with the new HEAD |
| ; This test verifies that OpLoopMerge instructions are properly placed as the second-to-last | ||
| ; instruction in their basic block, immediately preceding the branch instruction. | ||
|
|
||
| ; RUN: llvm-spirv %S/loopMerge.bc -o %t.spv |
There was a problem hiding this comment.
Please move .bc file content to the text and call llvm-as in the test before the translation.
| } else { | ||
| // If there's no proper terminator, add at the end | ||
| return addInstruction( | ||
| new SPIRVLoopMerge(MergeBlock, ContinueTarget, LoopControl, | ||
| LoopControlParameters, BB), | ||
| BB); | ||
| } |
There was a problem hiding this comment.
Probably clang-tidy will recommend to remove else as previous if terminates with return.
Co-authored-by: Dmitry Sidorov <[email protected]>
MrSidims
requested review from
YixingZhang007,
YuriPlyakhin,
maarquitos14 and
vmaksimo
| BB, const_cast<SPIRVInstruction *>(BB->getTerminateInstr())); | ||
| SPIRVInstruction *TermInst = const_cast<SPIRVInstruction *>(BB->getTerminateInstr()); | ||
| // OpLoopMerge must be the second-to-last instruction in the block, | ||
| // immediately preceding the branch instruction (OpBranch or OpBranchConditional) |
There was a problem hiding this comment.
| // immediately preceding the branch instruction (OpBranch or OpBranchConditional) | |
| // immediately preceding the branch instruction (OpBranch or OpBranchConditional). |
| LoopControlParameters, BB), | ||
| BB, TermInst); | ||
| } else { | ||
| // If there's no proper terminator, add at the end |
There was a problem hiding this comment.
| // If there's no proper terminator, add at the end | |
| // If there's no proper terminator, add at the end. |
There was a problem hiding this comment.
Also, is this correct? You mentioned that the spec requires this instruction to immediately precede either an OpBranch or an OpBranchConditional. My understanding is that, if none of these exist, this instruction could not be placed. Maybe we should assert here instead?
| const_cast<SPIRVInstruction *>(BB->getTerminateInstr())); | ||
| SPIRVInstruction *TermInst = const_cast<SPIRVInstruction *>(BB->getTerminateInstr()); | ||
| // OpLoopControlINTEL must be the second-to-last instruction in the block, | ||
| // immediately preceding the branch instruction (OpBranch or OpBranchConditional) |
There was a problem hiding this comment.
| // immediately preceding the branch instruction (OpBranch or OpBranchConditional) | |
| // immediately preceding the branch instruction (OpBranch or OpBranchConditional). |
| new SPIRVLoopControlINTEL(LoopControl, LoopControlParameters, BB), BB, | ||
| TermInst); | ||
| } else { | ||
| // If there's no proper terminator, add at the end |
There was a problem hiding this comment.
| // If there's no proper terminator, add at the end | |
| // If there's no proper terminator, add at the end. |
There was a problem hiding this comment.
Same than earlier. Is this compliant with the spec? Should we assert/report error instead?
| ; This test verifies that OpLoopMerge instructions are properly placed as the second-to-last | ||
| ; instruction in their basic block, immediately preceding the branch instruction. | ||
|
|
||
| ; RUN: llvm-spirv %S/loopMerge.bc -o %t.spv |
|
|
||
| ; RUN: llvm-spirv --to-text %t.spv -o - | FileCheck %s --check-prefix=CHECK-SPIRV-TEXT | ||
| ; CHECK-SPIRV-TEXT: LoopMerge | ||
| ; CHECK-SPIRV-TEXT-NEXT: BranchConditional No newline at end of file |
There was a problem hiding this comment.
Please, add newline at end of file.
4 participants
The SPIR-V specification requires that OpLoopMerge instructions must be placed as the second-to-last instruction in their basic block, immediately preceding the branch instruction (OpBranch or OpBranchConditional). The original implementation was not ensuring this correct placement, which could lead to SPIR-V validation failures.