[llvm] r369789 - [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 27 12:29:08 PDT 2019
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.
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 <mailto: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
>> <mailto: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 <mailto: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/732ed83f/attachment.html>
More information about the llvm-commits
mailing list