llvm dev - Jan 2011 - [LLVMdev] CodeExtractor.cpp potential bug?

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20101231/e7935dc7/attachment.html>

On Dec 31, 2010, at 11:20 AM, Vu Le wrote:
> 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.

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

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

On Mon, Jan 3, 2011 at 3:25 PM, Cameron Zwarich <zwarich at apple.com>
wrote:
> On Dec 31, 2010, at 11:20 AM, Vu Le wrote:
>
> > 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.
>
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110104/13e6e6e6/attachment.html>

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

On Sat, Jan 22, 2011 at 10:28 PM, Vu Le <vmle at ucdavis.edu> wrote:
> Hi Cameron,
> 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.
> Thanks,
> Vu
>
>
> On Mon, Jan 3, 2011 at 1:25 PM, Cameron Zwarich <zwarich at
apple.com> wrote:
>
>> On Dec 31, 2010, at 11:20 AM, Vu Le wrote:
>>
>> > 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.
>>
>>
>> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110124/6266c0ec/attachment.html>