[llvm] r369789 - [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null
Jordan Rupprecht via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 27 12:39:37 PDT 2019
On Tue, Aug 27, 2019 at 12:29 PM Philip Reames <listmail at philipreames.com>
wrote:
> Well, your problem is that your code is undefined.
>
> All inbounds pointers derived from a non-null base are non-null. (By
> assumption, they can't wrap.) And if you have a null base, then there are
> no inbounds derived pointers other than null itself. So, in the case
> below, the result of isValid is very likely to be poison which means
> branching on it is UB. The only non-poison result would be if %1 !=
> nullptr.
>
> Philip
>
> So the only case which can fail is when %1 is null.
>
Turns out that's the case -- when I print out the corresponding var for %1,
it's nullptr. So the pointer is calculated as base+offset, but IsValid() is
essentially only looking at base.
I'm not sure I understand the rest -- I'll try distilling the test case
into some more.
> On 8/27/19 11:19 AM, Jordan Rupprecht wrote:
>
> Yes, it's inbounds.
>
> Still narrowing it down (I spent all day yesterday just finding the file
> with the suspected miscompile). I'm snipping out a ton, but the
> relevant bits in the .ll seems to be:
>
> define doSomething() {
> entry:
> %1 = load i8*, i8** %<internal/mangled name of foo.x>, align 8
> %8 = load i64, i64* %offset31.i, align 1
> ...
> while.body.lr.ph.i:
> br label %while.body.i
> while.body.i:
> %9 = phi i64 [ %8, %while.body.lr.ph.i ], [ %11, %if.end.i ]
> %add.ptr.i = getelementptr inbounds i8, i8* %1, i64 %9
> ...
> if.end.i:
> %11 = load i64, i64* %offset.i, align 1
> ...
> %tobool.i15 = icmp eq i8* %add.ptr.i, null ; <-- relevant inlined
> IsValid() check
> }
>
> => that last check is now:
> %tobool.i15 = icmp eq i8* %1, null
>
> (the "valid pointer" is a base+offset, which I guess is why it got the
> name %add.ptr.i, if that's at all relevant)
>
> I've also confirmed it's these two patches causing the failure, and not
> any regressions in the broken window -- i.e. the test fails at r369795, but
> passes if I take r369788 as the baseline and then apply r369790 - r369794.
>
> On Mon, Aug 26, 2019 at 9:53 PM Philip Reames <listmail at philipreames.com>
> wrote:
>
>> Is there any chance <valid_pointer> is a inbounds geps? Without that, I
>> can't see the connection to the patch.
>>
>> Philip
>> On 8/26/19 5:05 PM, Jordan Rupprecht wrote:
>>
>> btw, we're seeing something that looks like miscompiles with this patch
>> (& after the fix in r369795). The code looks pretty well defined (passes
>> asan/msan/etc.).
>>
>> I'm trying to reduce it into something that actually triggers the bug,
>> but it vaguely looks like:
>> struct Foo {
>> bool IsValid() const { return x; }
>> Foo(char *y): x(y) {}
>> char *x;
>> }
>>
>> Foo get() {
>> return some_condition ? Foo(<valid pointer>) : Foo(nullptr);
>> }
>>
>> void doSomething() {
>> Foo foo = get();
>> if (foo.IsValid()) { .. }
>> }
>>
>> What I'm observing is get() go down the branch of some_condition being
>> true and returning Foo(<valid pointer>), but foo.IsValid() is returning
>> false.
>>
>> Given that context, is there anything you might see is a subtle bug with
>> this patch?
>>
>> On Fri, Aug 23, 2019 at 10:57 AM Philip Reames via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: reames
>>> Date: Fri Aug 23 10:58:58 2019
>>> New Revision: 369789
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=369789&view=rev
>>> Log:
>>> [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P,
>>> null
>>>
>>> This generalizes the isGEPKnownNonNull rule from ValueTracking to apply
>>> when we do not know if the base is non-null, and thus need to replace one
>>> condition with another.
>>>
>>> The core notion is that since an inbounds GEP can only form null if the
>>> base pointer is null and the offset is zero. However, if the offset is
>>> non-zero, the the "inbounds" marker makes the result poison. Thus, we're
>>> free to ignore the case where the offset is non-zero. Similarly, there's no
>>> case under which a non-null base can result in a null result without
>>> generating poison.
>>>
>>> Differential Revision: https://reviews.llvm.org/D66608
>>>
>>>
>>> Added:
>>> llvm/trunk/test/Transforms/InstCombine/gep-inbounds-null.ll
>>> Modified:
>>> llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>>>
>>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=369789&r1=369788&r2=369789&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>>> (original)
>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Fri
>>> Aug 23 10:58:58 2019
>>> @@ -894,6 +894,27 @@ Instruction *InstCombiner::foldGEPICmp(G
>>> Offset = EmitGEPOffset(GEPLHS);
>>> return new ICmpInst(ICmpInst::getSignedPredicate(Cond), Offset,
>>> Constant::getNullValue(Offset->getType()));
>>> + } else if (GEPLHS->isInBounds() && ICmpInst::isEquality(Cond) &&
>>> + GEPLHS->getType()->isPointerTy() && // TODO: extend to
>>> vector geps
>>> + isa<Constant>(RHS) && cast<Constant>(RHS)->isNullValue() &&
>>> + !NullPointerIsDefined(I.getFunction(),
>>> +
>>> RHS->getType()->getPointerAddressSpace())) {
>>> + // For most address spaces, an allocation can't be placed at null,
>>> but null
>>> + // itself is treated as a 0 size allocation in the in bounds
>>> rules. Thus,
>>> + // the only valid inbounds address derived from null, is null
>>> itself.
>>> + // Thus, we have four cases to consider:
>>> + // 1) Base == nullptr, Offset == 0 -> inbounds, null
>>> + // 2) Base == nullptr, Offset != 0 -> poison as the result is out
>>> of bounds
>>> + // 3) Base != nullptr, Offset == (-base) -> poison (crossing
>>> allocations)
>>> + // 4) Base != nullptr, Offset != (-base) -> nonnull (and possibly
>>> poison)
>>> + //
>>> + // (Note if we're indexing a type of size 0, that simply collapses
>>> into one
>>> + // of the buckets above.)
>>> + //
>>> + // In general, we're allowed to make values less poison (i.e. remove
>>> + // sources of full UB), so in this case, we just select between
>>> the two
>>> + // non-poison cases (1 and 4 above).
>>> + return new ICmpInst(Cond, GEPLHS->getPointerOperand(), RHS);
>>> } else if (GEPOperator *GEPRHS = dyn_cast<GEPOperator>(RHS)) {
>>> // If the base pointers are different, but the indices are the
>>> same, just
>>> // compare the base pointer.
>>>
>>> Added: llvm/trunk/test/Transforms/InstCombine/gep-inbounds-null.ll
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/gep-inbounds-null.ll?rev=369789&view=auto
>>>
>>> ==============================================================================
>>> --- llvm/trunk/test/Transforms/InstCombine/gep-inbounds-null.ll (added)
>>> +++ llvm/trunk/test/Transforms/InstCombine/gep-inbounds-null.ll Fri Aug
>>> 23 10:58:58 2019
>>> @@ -0,0 +1,184 @@
>>> +; NOTE: Assertions have been autogenerated by
>>> utils/update_test_checks.py
>>> +; RUN: opt -S < %s -instcombine | FileCheck %s
>>> +
>>> +;; Start by showing the results of constant folding (which doesn't use
>>> +;; the poison implied by gep for the nonnull cases).
>>> +
>>> +define i1 @test_ne_constants_null() {
>>> +; CHECK-LABEL: @test_ne_constants_null(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: ret i1 false
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, i8* null, i64 0
>>> + %cnd = icmp ne i8* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +
>>> +define i1 @test_ne_constants_nonnull() {
>>> +; CHECK-LABEL: @test_ne_constants_nonnull(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: ret i1 true
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, i8* null, i64 1
>>> + %cnd = icmp ne i8* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +
>>> +define i1 @test_eq_constants_null() {
>>> +; CHECK-LABEL: @test_eq_constants_null(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: ret i1 true
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, i8* null, i64 0
>>> + %cnd = icmp eq i8* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +
>>> +define i1 @test_eq_constants_nonnull() {
>>> +; CHECK-LABEL: @test_eq_constants_nonnull(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: ret i1 false
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, i8* null, i64 1
>>> + %cnd = icmp eq i8* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +
>>> +;; Then show the results for non-constants. These use the inbounds
>>> provided
>>> +;; UB fact to ignore the possible overflow cases.
>>> +
>>> +define i1 @test_ne(i8* %base, i64 %idx) {
>>> +; CHECK-LABEL: @test_ne(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: [[CND:%.*]] = icmp ne i8* [[BASE:%.*]], null
>>> +; CHECK-NEXT: ret i1 [[CND]]
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, i8* %base, i64 %idx
>>> + %cnd = icmp ne i8* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +
>>> +define i1 @test_eq(i8* %base, i64 %idx) {
>>> +; CHECK-LABEL: @test_eq(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: [[CND:%.*]] = icmp eq i8* [[BASE:%.*]], null
>>> +; CHECK-NEXT: ret i1 [[CND]]
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, i8* %base, i64 %idx
>>> + %cnd = icmp eq i8* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +
>>> +;; TODO: vectors not yet handled
>>> +define <2 x i1> @test_vector_base(<2 x i8*> %base, i64 %idx) {
>>> +; CHECK-LABEL: @test_vector_base(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds i8, <2 x i8*>
>>> [[BASE:%.*]], i64 [[IDX:%.*]]
>>> +; CHECK-NEXT: [[CND:%.*]] = icmp eq <2 x i8*> [[GEP]],
>>> zeroinitializer
>>> +; CHECK-NEXT: ret <2 x i1> [[CND]]
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, <2 x i8*> %base, i64 %idx
>>> + %cnd = icmp eq <2 x i8*> %gep, zeroinitializer
>>> + ret <2 x i1> %cnd
>>> +}
>>> +
>>> +define <2 x i1> @test_vector_index(i8* %base, <2 x i64> %idx) {
>>> +; CHECK-LABEL: @test_vector_index(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds i8, i8*
>>> [[BASE:%.*]], <2 x i64> [[IDX:%.*]]
>>> +; CHECK-NEXT: [[CND:%.*]] = icmp eq <2 x i8*> [[GEP]],
>>> zeroinitializer
>>> +; CHECK-NEXT: ret <2 x i1> [[CND]]
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, i8* %base, <2 x i64> %idx
>>> + %cnd = icmp eq <2 x i8*> %gep, zeroinitializer
>>> + ret <2 x i1> %cnd
>>> +}
>>> +
>>> +;; These two show instsimplify's reasoning getting to the non-zero
>>> offsets
>>> +;; before instcombine does.
>>> +
>>> +define i1 @test_eq_pos_idx(i8* %base) {
>>> +; CHECK-LABEL: @test_eq_pos_idx(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: ret i1 false
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, i8* %base, i64 1
>>> + %cnd = icmp eq i8* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +
>>> +define i1 @test_eq_neg_idx(i8* %base) {
>>> +; CHECK-LABEL: @test_eq_neg_idx(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: ret i1 false
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, i8* %base, i64 -1
>>> + %cnd = icmp eq i8* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +
>>> +;; Show an example with a zero sized type since that's
>>> +;; a cornercase which keeps getting mentioned. The GEP
>>> +;; produces %base regardless of the value of the index
>>> +;; expression.
>>> +define i1 @test_size0({}* %base, i64 %idx) {
>>> +; CHECK-LABEL: @test_size0(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: [[CND:%.*]] = icmp ne {}* [[BASE:%.*]], null
>>> +; CHECK-NEXT: ret i1 [[CND]]
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds {}, {}* %base, i64 %idx
>>> + %cnd = icmp ne {}* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +define i1 @test_size0_nonzero_offset({}* %base) {
>>> +; CHECK-LABEL: @test_size0_nonzero_offset(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: [[CND:%.*]] = icmp ne {}* [[BASE:%.*]], null
>>> +; CHECK-NEXT: ret i1 [[CND]]
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds {}, {}* %base, i64 15
>>> + %cnd = icmp ne {}* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +
>>> +
>>> +;; Finally, some negative tests for sanity checking.
>>> +
>>> +define i1 @neq_noinbounds(i8* %base, i64 %idx) {
>>> +; CHECK-LABEL: @neq_noinbounds(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: [[GEP:%.*]] = getelementptr i8, i8* [[BASE:%.*]], i64
>>> [[IDX:%.*]]
>>> +; CHECK-NEXT: [[CND:%.*]] = icmp ne i8* [[GEP]], null
>>> +; CHECK-NEXT: ret i1 [[CND]]
>>> +;
>>> +entry:
>>> + %gep = getelementptr i8, i8* %base, i64 %idx
>>> + %cnd = icmp ne i8* %gep, null
>>> + ret i1 %cnd
>>> +}
>>> +
>>> +define i1 @neg_objectatnull(i8 addrspace(2)* %base, i64 %idx) {
>>> +; CHECK-LABEL: @neg_objectatnull(
>>> +; CHECK-NEXT: entry:
>>> +; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds i8, i8
>>> addrspace(2)* [[BASE:%.*]], i64 [[IDX:%.*]]
>>> +; CHECK-NEXT: [[CND:%.*]] = icmp eq i8 addrspace(2)* [[GEP]], null
>>> +; CHECK-NEXT: ret i1 [[CND]]
>>> +;
>>> +entry:
>>> + %gep = getelementptr inbounds i8, i8 addrspace(2)* %base, i64 %idx
>>> + %cnd = icmp eq i8 addrspace(2)* %gep, null
>>> + ret i1 %cnd
>>> +}
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190827/cf84dbc7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4849 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190827/cf84dbc7/attachment.bin>
More information about the llvm-commits
mailing list