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

[Eli Friedman]

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:
>>> On Oct 17, 2012, at 5:01 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>>>
>>>> On Wed, Oct 17, 2012 at 4:54 PM, Bill Wendling <isanbard at gmail.com> wrote:
>>>>> Author: void
>>>>> Date: Wed Oct 17 18:54:19 2012
>>>>> New Revision: 166157
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=166157&view=rev
>>>>> Log:
>>>>> Check that the operand of the GEP is not the GEP itself. This occurred during an LTO build of LLVM.
>>>>>
>>>>> Modified:
>>>>>   llvm/trunk/lib/VMCore/Verifier.cpp
>>>>>
>>>>> Modified: llvm/trunk/lib/VMCore/Verifier.cpp
>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/VMCore/Verifier.cpp?rev=166157&r1=166156&r2=166157&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/VMCore/Verifier.cpp (original)
>>>>> +++ llvm/trunk/lib/VMCore/Verifier.cpp Wed Oct 17 18:54:19 2012
>>>>> @@ -1371,9 +1371,11 @@
>>>>>  Type *TargetTy = GEP.getPointerOperandType()->getScalarType();
>>>>>
>>>>>  Assert1(isa<PointerType>(TargetTy),
>>>>> -    "GEP base pointer is not a vector or a vector of pointers", &GEP);
>>>>> +          "GEP base pointer is not a vector or a vector of pointers", &GEP);
>>>>>  Assert1(cast<PointerType>(TargetTy)->getElementType()->isSized(),
>>>>>          "GEP into unsized type!", &GEP);
>>>>> +  Assert1(GEP.getPointerOperand() != &GEP,
>>>>> +          "GEP is using the result as the pointer operand!", &GEP);
>>>>
>>>> This check is bogus.  The following is legal IR:
>>>>
>>>> define void @f() {
>>>> ret void
>>>>
>>>> notreachedbb:
>>>> %xx = getelementptr i8* %xx, i32 0
>>>> ret void
>>>> }
>>>>
>>> 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.

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

-Eli