[llvm] r229029 - [IC] Fix a bug with the instcombine canonicalizing of loads and

[Hans Wennborg]

Merged in r229036.

On Thu, Feb 12, 2015 at 6:41 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> I already mentioned this to Hans, but for completeness, this also needs to
> go into 3.6.
>
> On Thu, Feb 12, 2015 at 6:30 PM, Chandler Carruth <chandlerc at gmail.com>
> wrote:
>>
>> Author: chandlerc
>> Date: Thu Feb 12 20:30:01 2015
>> New Revision: 229029
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=229029&view=rev
>> Log:
>> [IC] Fix a bug with the instcombine canonicalizing of loads and
>> propagating of metadata.
>>
>> We were propagating !nonnull metadata even when the newly formed load is
>> no longer of a pointer type. This is clearly broken and results in LLVM
>> failing the verifier and aborting. This patch just restricts the
>> propagation of !nonnull metadata to when we actually have a pointer
>> type.
>>
>> This bug report and the initial version of this patch was provided by
>> Charles Davis! Many thanks for finding this!
>>
>> We still need to add logic to round-trip the metadata correctly if we
>> combine from pointer types to integer types and then back by using range
>> metadata for the integer type loads. But this is the minimal and safe
>> version of the patch, which is important so we can backport it into 3.6.
>>
>> Modified:
>>     llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>>     llvm/trunk/test/Transforms/InstCombine/loadstore-metadata.ll
>>
>> Modified:
>> llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=229029&r1=229028&r2=229029&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>> (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>> Thu Feb 12 20:30:01 2015
>> @@ -330,11 +330,17 @@ static LoadInst *combineLoadToNewType(In
>>      case LLVMContext::MD_noalias:
>>      case LLVMContext::MD_nontemporal:
>>      case LLVMContext::MD_mem_parallel_loop_access:
>> -    case LLVMContext::MD_nonnull:
>>        // All of these directly apply.
>>        NewLoad->setMetadata(ID, N);
>>        break;
>>
>> +    case LLVMContext::MD_nonnull:
>> +      // FIXME: We should translate this into range metadata for integer
>> types
>> +      // and vice versa.
>> +      if (NewTy->isPointerTy())
>> +        NewLoad->setMetadata(ID, N);
>> +      break;
>> +
>>      case LLVMContext::MD_range:
>>        // FIXME: It would be nice to propagate this in some way, but the
>> type
>>        // conversions make it hard.
>> @@ -377,13 +383,14 @@ static StoreInst *combineStoreToNewValue
>>      case LLVMContext::MD_noalias:
>>      case LLVMContext::MD_nontemporal:
>>      case LLVMContext::MD_mem_parallel_loop_access:
>> -    case LLVMContext::MD_nonnull:
>>        // All of these directly apply.
>>        NewStore->setMetadata(ID, N);
>>        break;
>>
>>      case LLVMContext::MD_invariant_load:
>> +    case LLVMContext::MD_nonnull:
>>      case LLVMContext::MD_range:
>> +      // These don't apply for stores.
>>        break;
>>      }
>>    }
>>
>> Modified: llvm/trunk/test/Transforms/InstCombine/loadstore-metadata.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/loadstore-metadata.ll?rev=229029&r1=229028&r2=229029&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/InstCombine/loadstore-metadata.ll
>> (original)
>> +++ llvm/trunk/test/Transforms/InstCombine/loadstore-metadata.ll Thu Feb
>> 12 20:30:01 2015
>> @@ -1,5 +1,7 @@
>>  ; RUN: opt -instcombine -S < %s | FileCheck %s
>>
>> +target datalayout = "e-m:e-p:64:64:64-i64:64-f80:128-n8:16:32:64-S128"
>> +
>>  define i32 @test_load_cast_combine_tbaa(float* %ptr) {
>>  ; Ensure (cast (load (...))) -> (load (cast (...))) preserves TBAA.
>>  ; CHECK-LABEL: @test_load_cast_combine_tbaa(
>> @@ -78,6 +80,23 @@ exit:
>>    ret void
>>  }
>>
>> +define void @test_load_cast_combine_nonnull(float** %ptr) {
>> +; We can't preserve nonnull metadata when converting a load of a pointer
>> to
>> +; a load of an integer.
>> +; FIXME: We should really transform this into range metadata and vice
>> versa.
>> +;
>> +; CHECK-LABEL: @test_load_cast_combine_nonnull(
>> +; CHECK: %[[V:.*]] = load i64* %{{.*}}
>> +; CHECK-NOT: !nonnull
>> +; CHECK: store i64 %[[V]], i64*
>> +entry:
>> +  %p = load float** %ptr, !nonnull !3
>> +  %gep = getelementptr float** %ptr, i32 42
>> +  store float* %p, float** %gep
>> +
>> +  ret void
>> +}
>> +
>>  !0 = !{ !1, !1, i64 0 }
>>  !1 = !{ !1 }
>>  !2 = !{ !2, !1 }
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>