Hello, I was investigating GVN.cpp file and I found suspicious part:
1587 bool NeedToSplitEdges = false;
1588 for (pred_iterator PI = pred_begin(LoadBB), E = pred_end(LoadBB);
1589 PI != E; ++PI) {
1590 BasicBlock *Pred = *PI;
1591 if (IsValueFullyAvailableInBlock(Pred, FullyAvailableBlocks)) {
1592 continue;
1593 }
1594 PredLoads[Pred] = 0;
1595
1596 if (Pred->getTerminator()->getNumSuccessors() != 1) {
1597 if (isa<IndirectBrInst>(Pred->getTerminator())) {
1598 DEBUG(dbgs() << "COULD NOT PRE LOAD BECAUSE OF INDBR
CRITICAL EDGE '"
1599 << Pred->getName() << "': "
<< *LI << '\n');
1600 return false;
1601 }
1602 unsigned SuccNum = GetSuccessorNumber(Pred, LoadBB);
1603 toSplit.push_back(std::make_pair(Pred->getTerminator(), SuccNum));
1604 NeedToSplitEdges = true;
1605 }
1606 }
1607 if (NeedToSplitEdges)
1608 return false;
Because it iterates over predecessors it may happen that not the first one has
IndirectBrInst terminator.
However some "toSplit" edges can be already added. Is this OK?
Maybe I just misunderstood something?
Regards
--
Jakub Staszak
On May 4, 2010, at 9:41 AM, Jakub Staszak wrote:> Hello, I was investigating GVN.cpp file and I found suspicious part: > > 1587 bool NeedToSplitEdges = false; > 1588 for (pred_iterator PI = pred_begin(LoadBB), E = pred_end(LoadBB); > 1589 PI != E; ++PI) { > 1590 BasicBlock *Pred = *PI; > 1591 if (IsValueFullyAvailableInBlock(Pred, FullyAvailableBlocks)) { > 1592 continue; > 1593 } > 1594 PredLoads[Pred] = 0; > 1595 > 1596 if (Pred->getTerminator()->getNumSuccessors() != 1) { > 1597 if (isa<IndirectBrInst>(Pred->getTerminator())) { > 1598 DEBUG(dbgs() << "COULD NOT PRE LOAD BECAUSE OF INDBR CRITICAL EDGE '" > 1599 << Pred->getName() << "': " << *LI << '\n'); > 1600 return false; > 1601 } > 1602 unsigned SuccNum = GetSuccessorNumber(Pred, LoadBB); > 1603 toSplit.push_back(std::make_pair(Pred->getTerminator(), SuccNum)); > 1604 NeedToSplitEdges = true; > 1605 } > 1606 } > 1607 if (NeedToSplitEdges) > 1608 return false; > > Because it iterates over predecessors it may happen that not the first one has IndirectBrInst terminator. > However some "toSplit" edges can be already added. Is this OK? > Maybe I just misunderstood something?I suppose it might end up splitting some edges unnecessarily. That isn't a huge problem but it would be good to fix. The edges to be split could be saved on a local list and added to "toSplit" only after looking at all the predecessors. I'll fix that.
On May 4, 2010, at 9:41 AM, Jakub Staszak wrote:> > Because it iterates over predecessors it may happen that not the first one has IndirectBrInst terminator. > However some "toSplit" edges can be already added. Is this OK? > Maybe I just misunderstood something?It might result in doing some unnecessary edge splitting, but it's not dangerous in the "breaking the program" sense. Presumably SimplifyCFG will fold these blocks away if they're empty. --Owen