[llvm-commits] [PATCH] SuccIterator on bbs without terminator insts

[Tobias Grosser]

On 07/03/2011 07:17 PM, Nick Lewycky wrote:
> Tobias Grosser wrote:
>> Hi,
>>
>> this patch removes the assert, that triggers if SuccIterator is
>> constructed for a basic block without a terminator instruction. Instead
>> of triggering an assert a succ_end() iterator is returned. This models a
>> basic block with zero successors and allows us to use F->viewCFG() on
>> incompletely constructed functions.
>>
>> OK to commit?
>
> Intuitively no, because all basic blocks are required to have
> terminators. But I suspect you ran into this for a reason, and tools
> like succ_begin/end() may not be able to assume that the CFG is valid at
> any point in time because the IR is being modified. Could you explain
> more about what you were doing which triggered this?

It's pretty easy.

In Polly we modify the CFG by adding some additional basic blocks 
(actually a branch that contains an optimized code path). Once in a 
while (because of a bug in Polly or somewhere in LLVM) I have a failing 
test case. My first step to investigate a bug is to put a breakpoint at 
the place in the code where I suspect the bug (or an assert triggered). 
My next step is to call F->viewCFG() to see the current state of the 
CFG. Unfortunately this almost never works, as the CFG is at this point 
not yet fully constructed and consequently the above assert triggers.

In general I believe for debugging it would be very helpful if 
F->viewCFG() would already work during the construction of the CFG.

 > Would it be
 > difficult to avoid calling succ_begin() on malformed basic blocks?

Yes. The problem is viewCFG() and in general the Graph printing 
infrastructure use the normal GraphTraits description of the CFG, which 
again calls the succ_begin/end() functions. Hence, viewCFG() only works 
on incomplete functions/basic blocks if the normal GraphTraits and the 
succ_begin/end() iterators can handle incomplete basic blocks. This is 
currently not the case (because of the assert).

I personally see no problem removing the assert, because conceptually a 
basic block without any terminator is just a basic block with zero 
successors. There is also no need to enforce the required form of the 
LLVM-IR through the succ_begin()/end() iterators, as we have a dedicated 
verification pass for this.

Cheers
Tobi