Skip to content

Commit

Permalink
Ensure that PHI node has an incoming value per each predecessor insta…
Browse files Browse the repository at this point in the history
…nce (KhronosGroup#2736)

This PR partially fixes issue KhronosGroup#2702 in the part that is responsible for SPIR-V to LLVM IR translation. Namely, this PR ensures that all PHI nodes of a Function has the number of incoming blocks matching block's predecessor count. When a PHI node doesn't conform to this rule, this PR inserts missing number of (Value, Basic Block) pairs to make the PHI node valid.

Another problem from KhronosGroup#2702, that is violation of the requirement to OpPhi's to have exactly one Parent ID operand for each parent block of the current block in the CFG in the output SPIR-V code, is out of scope of this PR.
  • Loading branch information
VyacheslavLevytskyy authored and MrSidims committed Nov 7, 2024
1 parent 0d1b803 commit e19618f
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 0 deletions.
41 changes: 41 additions & 0 deletions lib/SPIRV/SPIRVReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3036,6 +3036,46 @@ void SPIRVToLLVM::transFunctionAttrs(SPIRVFunction *BF, Function *F) {
});
}

namespace {
// One basic block can be a predecessor to another basic block more than
// once (https://github.com/KhronosGroup/SPIRV-LLVM-Translator/issues/2702).
// This function fixes any PHIs that break this rule.
static void validatePhiPredecessors(Function *F) {
for (BasicBlock &BB : *F) {
bool UniquePreds = true;
DenseMap<BasicBlock *, unsigned> PredsCnt;
for (BasicBlock *PredBB : predecessors(&BB)) {
auto It = PredsCnt.try_emplace(PredBB, 1);
if (!It.second) {
UniquePreds = false;
++It.first->second;
}
}
if (UniquePreds)
continue;
// `phi` requires an incoming value per each predecessor instance, even
// it's the same basic block that has been already inserted as an incoming
// value of the `phi`.
for (PHINode &Phi : BB.phis()) {
SmallVector<Value *> Vs;
SmallVector<BasicBlock *> Bs;
for (auto [V, B] : zip(Phi.incoming_values(), Phi.blocks())) {
unsigned N = PredsCnt[B];
Vs.insert(Vs.end(), N, V);
Bs.insert(Bs.end(), N, B);
}
unsigned I = 0;
for (unsigned N = Phi.getNumIncomingValues(); I < N; ++I) {
Phi.setIncomingValue(I, Vs[I]);
Phi.setIncomingBlock(I, Bs[I]);
}
for (unsigned N = Vs.size(); I < N; ++I)
Phi.addIncoming(Vs[I], Bs[I]);
}
}
}
} // namespace

Function *SPIRVToLLVM::transFunction(SPIRVFunction *BF, unsigned AS) {
auto Loc = FuncMap.find(BF);
if (Loc != FuncMap.end())
Expand Down Expand Up @@ -3121,6 +3161,7 @@ Function *SPIRVToLLVM::transFunction(SPIRVFunction *BF, unsigned AS) {
}
}

validatePhiPredecessors(F);
transLLVMLoopMetadata(F);

return F;
Expand Down
37 changes: 37 additions & 0 deletions test/phi-multiple-predecessors.spvasm
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
; REQUIRES: spirv-as
; RUN: spirv-as --target-env spv1.0 -o %t.spv %s
; RUN: spirv-val %t.spv
; RUN: llvm-spirv -r -o - %t.spv | llvm-dis | FileCheck %s

OpCapability Kernel
OpCapability Addresses
OpCapability Int64
OpCapability Linkage
%1 = OpExtInstImport "OpenCL.std"
OpMemoryModel Physical64 OpenCL
OpSource OpenCL_CPP 100000
OpName %foo "foo"
OpName %get "get"
OpDecorate %foo LinkageAttributes "foo" Export
OpDecorate %get LinkageAttributes "get" Import
%uint = OpTypeInt 32 0
%3 = OpTypeFunction %uint
%ulong = OpTypeInt 64 0
%uint_2 = OpConstant %uint 2
%uint_4 = OpConstant %uint 4
%get = OpFunction %uint None %3
OpFunctionEnd
%foo = OpFunction %uint None %3
%11 = OpLabel
%9 = OpFunctionCall %uint %get
OpSwitch %9 %12 10 %13 4 %13 0 %13 42 %13
%12 = OpLabel
OpBranch %13
%13 = OpLabel
%10 = OpPhi %uint %uint_4 %11 %uint_2 %12
%14 = OpPhi %uint %uint_2 %12 %uint_4 %11
OpReturnValue %14
OpFunctionEnd

; CHECK: phi i32 [ 4, %0 ], [ 4, %0 ], [ 4, %0 ], [ 4, %0 ], [ 2, %2 ]
; CHECK: phi i32 [ 2, %2 ], [ 4, %0 ], [ 4, %0 ], [ 4, %0 ], [ 4, %0 ]

0 comments on commit e19618f

Please sign in to comment.