CodeExtractor.cpp potential bug?

Project Infrastructure LLVM Dev List Archives
#1

There might be a misuse of DominatorTree::splitBasicBlock in CodeExtractor.cpp, line 145.
Header is splited into two (OldPred->NewBB).

Line 145 updates the dominator tree by calling DT->splitBasicBlock(NewBB).
I think it should be DT->splitBasicBlock(OldPred).

When I tried to extract a code region whose header has 2 successors, my pass crashed.
It was because header (or OldPred) is the block that was splited, not NewBB.
DominatorTree::splitBasicBlock(BB) requires BB to have one successor.

Vu

#2

The code in Dominator::splitBasicBlock() looks correct, but I think the comment and assert may not be. I was writing a patch where I hit the same issue.

Cameron

#3

Thank you, Cameron.
I also think Dominator::splitBasicBlock() is kind of OK, except the assertion at line 232 (Dominator.h)
That assertion fails when we split the entry basic block (obviously, it does not have any dominator).

What I asked for was the use of splitBasicBlock in CodeExtractor.cpp, line 145.
Vu

#4

I forgot to cc llvmdev.
Here is my original message.

I’m a bit confused on DominatorTreeBase::Split() (http://llvm.org/docs/doxygen/html/Dominators_8h_source.html#l00229)
If a basic block A splits into A->B, when I call Split(), which is NewBB? A or B?

The semantics shows that NewBB is the newly split basic block B.
But the assertion at line 229 doesn’t seem right.

00229     assert(std::distance(GraphT::child_begin(NewBB),
00230                          GraphT::child_end(NewBB)) == 1 &&
00231            "NewBB should have a single successor!");

If A has 2 successors C, D, after it split to A->NewBB, NewBB should have 2 successors.
Hope anyone could explain this to me.
Thanks,
Vu