Skip to content

S
swiftshader
Commit ea15bbe7 by Jim Stichnoth
Options

Subzero: Fix a bug in advanced phi lowering.

When a temporary is introduced to break a cycle, we neglected to update the predecessor count for one of the operands, leading to a possible assertion failure. This problem isn't currently seen in master, but it arises when we enable register aliases, as in https://codereview.chromium.org/1427973003/ . No changes are seen in spec2k code generation as a result of this fix. A consistency check is added to help find future problems like this. Also, refactored iteration over the Phi descriptor array to use range-based for loops and avoid directly indexing the array. BUG= none R=jpp@chromium.org Review URL: https://codereview.chromium.org/1435543002 .
parent f49b2396
Showing with 100 additions and 87 deletions
src/IceCfgNode.cpp
...@@ -298,7 +298,31 @@ CfgNode *CfgNode::splitIncomingEdge(CfgNode *Pred, SizeT EdgeIndex) { ...@@ -298,7 +298,31 @@ CfgNode *CfgNode::splitIncomingEdge(CfgNode *Pred, SizeT EdgeIndex) {
namespace { namespace {
// Helper function used by advancedPhiLowering(). // Helpers for advancedPhiLowering().
class PhiDesc {
PhiDesc() = delete;
PhiDesc(const PhiDesc &) = delete;
PhiDesc &operator=(const PhiDesc &) = delete;
public:
PhiDesc(InstPhi *Phi, Variable *Dest) : Phi(Phi), Dest(Dest) {}
PhiDesc(PhiDesc &&) = default;
InstPhi *Phi = nullptr;
Variable *Dest = nullptr;
Operand *Src = nullptr;
bool Processed = false;
size_t NumPred = 0; // number of entries whose Src is this Dest
int32_t Weight = 0; // preference for topological order
};
using PhiDescList = llvm::SmallVector<PhiDesc, 32>;
// Always pick NumPred=0 over NumPred>0.
constexpr int32_t WeightNoPreds = 4;
// Prefer Src as a register because the register might free up.
constexpr int32_t WeightSrcIsReg = 2;
// Prefer Dest not as a register because the register stays free longer.
constexpr int32_t WeightDestNotReg = 1;
bool sameVarOrReg(TargetLowering *Target, const Variable *Var1, bool sameVarOrReg(TargetLowering *Target, const Variable *Var1,
const Operand *Opnd) { const Operand *Opnd) {
if (Var1 == Opnd) if (Var1 == Opnd)
...@@ -324,6 +348,18 @@ bool sameVarOrReg(TargetLowering *Target, const Variable *Var1, ...@@ -324,6 +348,18 @@ bool sameVarOrReg(TargetLowering *Target, const Variable *Var1,
return Target->getAliasesForRegister(RegNum1)[RegNum2]; return Target->getAliasesForRegister(RegNum1)[RegNum2];
} }
// Update NumPred for all Phi assignments using Var as their Dest variable.
// Also update Weight if NumPred dropped from 1 to 0.
void updatePreds(PhiDescList &Desc, TargetLowering *Target, Variable *Var) {
for (PhiDesc &Item : Desc) {
if (!Item.Processed && sameVarOrReg(Target, Var, Item.Dest)) {
if (--Item.NumPred == 0) {
Item.Weight += WeightNoPreds;
}
}
}
}
} // end of anonymous namespace } // end of anonymous namespace
// This the "advanced" version of Phi lowering for a basic block, in contrast // This the "advanced" version of Phi lowering for a basic block, in contrast
...@@ -368,53 +404,41 @@ void CfgNode::advancedPhiLowering() { ...@@ -368,53 +404,41 @@ void CfgNode::advancedPhiLowering() {
if (getPhis().empty()) if (getPhis().empty())
return; return;
// Count the number of non-deleted Phi instructions. PhiDescList Desc;
struct PhiDesc {
InstPhi *Phi;
Variable *Dest;
Operand *Src;
bool Processed;
size_t NumPred; // number of entries whose Src is this Dest
int32_t Weight; // preference for topological order
};
llvm::SmallVector<PhiDesc, 32> Desc(getPhis().size());
size_t NumPhis = 0;
for (Inst &I : Phis) { for (Inst &I : Phis) {
auto *Inst = llvm::dyn_cast<InstPhi>(&I); auto *Phi = llvm::dyn_cast<InstPhi>(&I);
if (!Inst->isDeleted()) { if (!Phi->isDeleted()) {
Variable *Dest = Inst->getDest(); Variable *Dest = Phi->getDest();
Desc[NumPhis].Phi = Inst; Desc.emplace_back(Phi, Dest);
Desc[NumPhis].Dest = Dest;
++NumPhis;
// Undo the effect of the phi instruction on this node's live-in set by // Undo the effect of the phi instruction on this node's live-in set by
// marking the phi dest variable as live on entry. // marking the phi dest variable as live on entry.
SizeT VarNum = Func->getLiveness()->getLiveIndex(Dest->getIndex()); SizeT VarNum = Func->getLiveness()->getLiveIndex(Dest->getIndex());
assert(!Func->getLiveness()->getLiveIn(this)[VarNum]); assert(!Func->getLiveness()->getLiveIn(this)[VarNum]);
Func->getLiveness()->getLiveIn(this)[VarNum] = true; Func->getLiveness()->getLiveIn(this)[VarNum] = true;
Inst->setDeleted(); Phi->setDeleted();
} }
} }
if (NumPhis == 0) if (Desc.empty())
return; return;
TargetLowering *Target = Func->getTarget(); TargetLowering *Target = Func->getTarget();
SizeT InEdgeIndex = 0; SizeT InEdgeIndex = 0;
for (CfgNode *Pred : InEdges) { for (CfgNode *Pred : InEdges) {
CfgNode *Split = splitIncomingEdge(Pred, InEdgeIndex++); CfgNode *Split = splitIncomingEdge(Pred, InEdgeIndex++);
SizeT Remaining = NumPhis; SizeT Remaining = Desc.size();
// First pass computes Src and initializes NumPred. // First pass computes Src and initializes NumPred.
for (size_t I = 0; I < NumPhis; ++I) { for (PhiDesc &Item : Desc) {
Variable *Dest = Desc[I].Dest; Variable *Dest = Item.Dest;
Operand *Src = Desc[I].Phi->getOperandForTarget(Pred); Operand *Src = Item.Phi->getOperandForTarget(Pred);
Desc[I].Src = Src; Item.Src = Src;
Desc[I].Processed = false; Item.Processed = false;
Desc[I].NumPred = 0; Item.NumPred = 0;
// Cherry-pick any trivial assignments, so that they don't contribute to // Cherry-pick any trivial assignments, so that they don't contribute to
// the running complexity of the topological sort. // the running complexity of the topological sort.
if (sameVarOrReg(Target, Dest, Src)) { if (sameVarOrReg(Target, Dest, Src)) {
Desc[I].Processed = true; Item.Processed = true;
--Remaining; --Remaining;
if (Dest != Src) if (Dest != Src)
// If Dest and Src are syntactically the same, don't bother adding // If Dest and Src are syntactically the same, don't bother adding
...@@ -424,90 +448,80 @@ void CfgNode::advancedPhiLowering() { ...@@ -424,90 +448,80 @@ void CfgNode::advancedPhiLowering() {
Split->appendInst(InstAssign::create(Func, Dest, Src)); Split->appendInst(InstAssign::create(Func, Dest, Src));
} }
} }
// Second pass computes NumPred by comparing every pair of Phi // Second pass computes NumPred by comparing every pair of Phi instructions.
// instructions. for (PhiDesc &Item : Desc) {
for (size_t I = 0; I < NumPhis; ++I) { if (Item.Processed)
if (Desc[I].Processed)
continue; continue;
const Variable *Dest = Desc[I].Dest; const Variable *Dest = Item.Dest;
for (size_t J = 0; J < NumPhis; ++J) { for (PhiDesc &Item2 : Desc) {
if (Desc[J].Processed) if (Item2.Processed)
continue; continue;
if (I != J) { // There shouldn't be two different Phis with the same Dest variable or
// There shouldn't be two Phis with the same Dest variable or
// register. // register.
assert(!sameVarOrReg(Target, Dest, Desc[J].Dest)); assert((&Item == &Item2) || !sameVarOrReg(Target, Dest, Item2.Dest));
} if (sameVarOrReg(Target, Dest, Item2.Src))
const Operand *Src = Desc[J].Src; ++Item.NumPred;
if (sameVarOrReg(Target, Dest, Src))
++Desc[I].NumPred;
} }
} }
// Another pass to compute initial Weight values. // Another pass to compute initial Weight values.
for (PhiDesc &Item : Desc) {
// Always pick NumPred=0 over NumPred>0. if (Item.Processed)
constexpr int32_t WeightNoPreds = 4;
// Prefer Src as a register because the register might free up.
constexpr int32_t WeightSrcIsReg = 2;
// Prefer Dest not as a register because the register stays free longer.
constexpr int32_t WeightDestNotReg = 1;
for (size_t I = 0; I < NumPhis; ++I) {
if (Desc[I].Processed)
continue; continue;
int32_t Weight = 0; int32_t Weight = 0;
if (Desc[I].NumPred == 0) if (Item.NumPred == 0)
Weight += WeightNoPreds; Weight += WeightNoPreds;
if (auto *Var = llvm::dyn_cast<Variable>(Desc[I].Src)) if (auto *Var = llvm::dyn_cast<Variable>(Item.Src))
if (Var->hasReg()) if (Var->hasReg())
Weight += WeightSrcIsReg; Weight += WeightSrcIsReg;
if (!Desc[I].Dest->hasReg()) if (!Item.Dest->hasReg())
Weight += WeightDestNotReg; Weight += WeightDestNotReg;
Desc[I].Weight = Weight; Item.Weight = Weight;
} }
// Repeatedly choose and process the best candidate in the topological // Repeatedly choose and process the best candidate in the topological sort,
// sort, until no candidates remain. This implementation is O(N^2) where N // until no candidates remain. This implementation is O(N^2) where N is the
// is the number of Phi instructions, but with a small constant factor // number of Phi instructions, but with a small constant factor compared to
// compared to a likely implementation of O(N) topological sort. // a likely implementation of O(N) topological sort.
for (; Remaining; --Remaining) { for (; Remaining; --Remaining) {
size_t BestIndex = 0;
int32_t BestWeight = -1; int32_t BestWeight = -1;
PhiDesc *BestItem = nullptr;
// Find the best candidate. // Find the best candidate.
for (size_t I = 0; I < NumPhis; ++I) { for (PhiDesc &Item : Desc) {
if (Desc[I].Processed) if (Item.Processed)
continue; continue;
int32_t Weight = 0; int32_t Weight = 0;
Weight = Desc[I].Weight; Weight = Item.Weight;
if (Weight > BestWeight) { if (Weight > BestWeight) {
BestIndex = I; BestItem = &Item;
BestWeight = Weight; BestWeight = Weight;
} }
} }
assert(BestWeight >= 0); assert(BestWeight >= 0);
assert(Desc[BestIndex].NumPred <= 1); assert(BestItem->NumPred <= 1);
Variable *Dest = Desc[BestIndex].Dest; Variable *Dest = BestItem->Dest;
Operand *Src = Desc[BestIndex].Src; Operand *Src = BestItem->Src;
assert(!sameVarOrReg(Target, Dest, Src)); assert(!sameVarOrReg(Target, Dest, Src));
// Break a cycle by introducing a temporary. // Break a cycle by introducing a temporary.
if (Desc[BestIndex].NumPred) { if (BestItem->NumPred) {
bool Found = false; bool Found = false;
// If the target instruction "A=B" is part of a cycle, find the "X=A" // If the target instruction "A=B" is part of a cycle, find the "X=A"
// assignment in the cycle because it will have to be rewritten as // assignment in the cycle because it will have to be rewritten as
// "X=tmp". // "X=tmp".
for (size_t J = 0; !Found && J < NumPhis; ++J) { for (PhiDesc &Item : Desc) {
if (Desc[J].Processed) if (Item.Processed)
continue; continue;
Operand *OtherSrc = Desc[J].Src; Operand *OtherSrc = Item.Src;
if (Desc[J].NumPred && sameVarOrReg(Target, Dest, OtherSrc)) { if (Item.NumPred && sameVarOrReg(Target, Dest, OtherSrc)) {
SizeT VarNum = Func->getNumVariables(); SizeT VarNum = Func->getNumVariables();
Variable *Tmp = Func->makeVariable(OtherSrc->getType()); Variable *Tmp = Func->makeVariable(OtherSrc->getType());
if (BuildDefs::dump()) if (BuildDefs::dump())
Tmp->setName(Func, "__split_" + std::to_string(VarNum)); Tmp->setName(Func, "__split_" + std::to_string(VarNum));
Split->appendInst(InstAssign::create(Func, Tmp, OtherSrc)); Split->appendInst(InstAssign::create(Func, Tmp, OtherSrc));
Desc[J].Src = Tmp; Item.Src = Tmp;
updatePreds(Desc, Target, llvm::cast<Variable>(OtherSrc));
Found = true; Found = true;
break;
} }
} }
assert(Found); assert(Found);
...@@ -515,24 +529,23 @@ void CfgNode::advancedPhiLowering() { ...@@ -515,24 +529,23 @@ void CfgNode::advancedPhiLowering() {
// Now that a cycle (if any) has been broken, create the actual // Now that a cycle (if any) has been broken, create the actual
// assignment. // assignment.
Split->appendInst(InstAssign::create(Func, Dest, Src)); Split->appendInst(InstAssign::create(Func, Dest, Src));
// Update NumPred for all Phi assignments using this Phi's Src as their if (auto *Var = llvm::dyn_cast<Variable>(Src))
// Dest variable. Also update Weight if NumPred dropped from 1 to 0. updatePreds(Desc, Target, Var);
if (auto *Var = llvm::dyn_cast<Variable>(Src)) { BestItem->Processed = true;
for (size_t I = 0; I < NumPhis; ++I) {
if (Desc[I].Processed)
continue;
if (sameVarOrReg(Target, Var, Desc[I].Dest)) {
if (--Desc[I].NumPred == 0)
Desc[I].Weight += WeightNoPreds;
}
}
}
Desc[BestIndex].Processed = true;
} }
Split->appendInst(InstBr::create(Func, this)); Split->appendInst(InstBr::create(Func, this));
Split->genCode(); Split->genCode();
Func->getVMetadata()->addNode(Split); Func->getVMetadata()->addNode(Split);
// Validate to be safe. All items should be marked as processed, and have
// no predecessors.
if (BuildDefs::asserts()) {
for (PhiDesc &Item : Desc) {
(void)Item;
assert(Item.Processed);
assert(Item.NumPred == 0);
}
}
} }
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment