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

[Eli Friedman]

On Wed, Oct 17, 2012 at 5:58 PM, Bill Wendling <isanbard at gmail.com> wrote:
> 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.

Yes. The issue is, the bookkeeping required to avoid such
transformations would be complicated and expensive in passes like jump
threading and SimplifyCFG.

> 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.

Essentially, yes.

-Eli