[llvm-commits] [llvm] r166157 - /llvm/trunk/lib/VMCore/Verifier.cpp

[Bill Wendling]

On Oct 17, 2012, at 5:47 PM, Eli Friedman <eli.friedman at gmail.com> wrote:

> On Wed, Oct 17, 2012 at 5:34 PM, Bill Wendling <isanbard at gmail.com> wrote:
>> 
>> n Oct 17, 2012, at 5:26 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>> 
>>> On Wed, Oct 17, 2012 at 5:03 PM, Bill Wendling <isanbard at gmail.com> wrote:
>>>> 
>>>> I disagree. The code you have there isn't in SSA form, which though there may be holes in the IR format which allows for such non-SSA form, LLVM itself wants to be in SSA form and shouldn't encourage this type of code.
>>> 
>>> By "In SSA form", I assume you mean respects the dominance properties
>>> of SSA.  According to LLVM's definition of dominance, %xx dominates
>>> %xx.
>>> 
>>> Code like that is a natural consequence of allowing unreachable basic
>>> blocks in IR.
>>> 
>> Oh. I didn't know that LLVM defines things in a non-standard way.
> 
> Conventional definitions of dominance are meaningless for unreachable
> blocks.  We therefore define it in the way which happens to be most
> convenient.
> 
You keep saying "we define" as if it's written down somewhere, but it's not.

>> Using something before it's defined is generally considered bad, if not downright impossible. I therefore maintain that LLVM is wrong, whether it's a "natural consequence" or not (though I have no idea what you mean by it being a "natural consequence").
> 
> Suppose you have a simple loop guarded by a branch.  We simplify the
> conditional branch into an unconditional branch, and now the loop is
> no longer reachable (but we don't eliminate the loop header because it
> still has a predecessor).  We then simplify the PHI nodes at the
> beginning of the loop.  We now have a GEP with itself as an operand.
> 
> It's not practical to ban the second transformation.  We could
> theoretically make the first transformation illegal; that's equivalent
> to banning unreachable blocks in IR.
> 
Doing transformations on unreachable blocks is useless of course. The fact that we do it is not a compelling argument for allowing instructions to be in non-SSA form. As far as I can tell from your description, it's a lack of cleaning up the blocks that were made unreachable that allows this form to creep in.

-bw