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