<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 03/19/2014 06:11 AM, Richard Osborne
wrote:<br>
</div>
<blockquote
cite="mid:differential-rev-PHID-DREV-2xjr6oszyo5yv6yagpma-req@llvm-reviews.chandlerc.com"
type="cite">
<pre wrap="">Previously the code didn't check if the before and after types for the
store were pointers to different address spaces. This resulted in
instcombine using a bitcast to convert between pointers to different
address spaces, causing an assertion due to the invalid cast.
It is not be appropriate to use addrspacecast this case because it is
not guaranteed to be a no-op cast. Instead bail out and do not do the
transformation.
<a class="moz-txt-link-freetext" href="http://llvm-reviews.chandlerc.com/D3117">http://llvm-reviews.chandlerc.com/D3117</a>
Files:
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
test/Transforms/InstCombine/bitcast-store.ll
Index: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
+++ lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
@@ -517,27 +517,35 @@
IC.getDataLayout()->getTypeSizeInBits(DestPTy))
return 0;
+ // Don't introduce casts between pointers to different address spaces. We
+ // can't use the addrspacecast instruction in this case since, depending on
+ // the target, addrspacecast may not be a no-op cast.
+ if (SrcPTy->isPointerTy() && DestPTy->isPointerTy() &&
+ cast<PointerType>(SrcPTy)->getAddressSpace() !=
+ cast<PointerType>(DestPTy)->getAddressSpace())
+ return 0;
+</pre>
</blockquote>
There is getPointerAddressSpace() instead of needing to use the
cast, but I think it's cleaner to just use dyn_cast instead of
checking if it's a pointer and then casting to pointer.<br>
<br>
Right above here is a check leftover from when bitcast was allowed
if the pointers were the same size. You should update that and
replace it with this.<br>
<blockquote
cite="mid:differential-rev-PHID-DREV-2xjr6oszyo5yv6yagpma-req@llvm-reviews.chandlerc.com"
type="cite">
<pre wrap="">
// Okay, we are casting from one integer or pointer type to another of
// the same size. Instead of casting the pointer before
// the store, cast the value to be stored.
Value *NewCast;
- Value *SIOp0 = SI.getOperand(0);
Instruction::CastOps opcode = Instruction::BitCast;
- Type* CastSrcTy = SIOp0->getType();
+ Type* CastSrcTy = DestPTy;
Type* CastDstTy = SrcPTy;
if (CastDstTy->isPointerTy()) {
if (CastSrcTy->isIntegerTy())
opcode = Instruction::IntToPtr;
} else if (CastDstTy->isIntegerTy()) {
- if (SIOp0->getType()->isPointerTy())
+ if (CastSrcTy->isPointerTy())
opcode = Instruction::PtrToInt;
}
// SIOp0 is a pointer to aggregate and this is a store to the first field,
// emit a GEP to index into its first field.
if (!NewGEPIndices.empty())
CastOp = IC.Builder->CreateInBoundsGEP(CastOp, NewGEPIndices);
+ Value *SIOp0 = SI.getOperand(0);
NewCast = IC.Builder->CreateCast(opcode, SIOp0, CastDstTy,
SIOp0->getName()+".c<a class="moz-txt-link-rfc2396E" href="mailto:);SI.setOperand(0,NewCast);Index:test/Transforms/InstCombine/bitcast-store.ll===================================================================---test/Transforms/InstCombine/bitcast-store.ll+++test/Transforms/InstCombine/bitcast-store.ll@@-3,19+3,33@@;Instcombineshouldpreservemetadataandalignmentwhile;foldingabitcastintoastore.-;CHECK:storei32(...)**bitcast(i8**getelementptrinbounds([5xi8*]*@G,i640,i642)toi32(...)**),i32(...)***%0,align16,!tag!0-targetdatalayout=">");
SI.setOperand(0, NewCast);
Index: test/Transforms/InstCombine/bitcast-store.ll
===================================================================
--- test/Transforms/InstCombine/bitcast-store.ll
+++ test/Transforms/InstCombine/bitcast-store.ll
@@ -3,19 +3,33 @@
; Instcombine should preserve metadata and alignment while
; folding a bitcast into a store.
-; CHECK: store i32 (...)** bitcast (i8** getelementptr inbounds ([5 x i8*]* @G, i64 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 16, !tag !0
-
target datalayout = "</a>e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
%struct.A = type { i32 (...)** }
@G = external constant [5 x i8*]
+; CHECK-LABEL: @foo
+; CHECK: store i32 (...)** bitcast (i8** getelementptr inbounds ([5 x i8*]* @G, i64 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 16, !tag !0
define void @foo(%struct.A* %a) nounwind {
entry:
%0 = bitcast %struct.A* %a to i8***
store i8** getelementptr inbounds ([5 x i8*]* @G, i64 0, i64 2), i8*** %0, align 16, !tag !0
ret void
}
!0 = metadata !{metadata !"hello"}
+
+; Check instcombine doesn't try and fold the following bitcast into the store.
+; This transformation would not be safe since we would need to use addrspacecast
+; and addrspacecast is not guaranteed to be a no-op cast.
+
+; CHECK-LABEL: @bar
+; CHECK: %cast = bitcast i8** %b to i8 addrspace(1)**
+; CHECK: store i8 addrspace(1)* %a, i8 addrspace(1)** %cast
+define void @bar(i8 addrspace(1)* %a, i8** %b) nounwind {
+entry:
+ %cast = bitcast i8** %b to i8 addrspace(1)**
+ store i8 addrspace(1)* %a, i8 addrspace(1)** %cast
+ ret void
+}
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</body>
</html>