<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 14, 2013 at 6:58 PM, Tom Stellard <span dir="ltr"><<a href="mailto:tom@stellard.net" target="_blank" class="cremed">tom@stellard.net</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Tom Stellard <<a href="mailto:thomas.stellard@amd.com" class="cremed">thomas.stellard@amd.com</a>><br>
<br>
Patch by: Michael Ferguson<br>
<br>
This is a modified version of Michael's patch posted here:<br>
<a href="http://www.llvm.org/bugs/show_bug.cgi?id=15907#c1" target="_blank" class="cremed">http://www.llvm.org/bugs/show_bug.cgi?id=15907#c1</a> I removed the bitcast<br>
wrapper and left only what was necessary to fix the bug.<br></blockquote><div><br></div><div>Thanks for cleaning this up and taking the time to mail it out to the list. A couple of comments:</div><div><br></div><div>Can you use Phabricator? Or at least mail as an attachment?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="http://www.llvm.org/bugs/show_bug.cgi?id=15907" target="_blank" class="cremed">http://www.llvm.org/bugs/show_bug.cgi?id=15907</a><br>
---<br>
lib/Transforms/Scalar/SROA.cpp | 9 ++++--<br>
test/Transforms/SROA/sroa-addrspace-bug.ll | 52 ++++++++++++++++++++++++++++++<br>
2 files changed, 59 insertions(+), 2 deletions(-)<br>
create mode 100644 test/Transforms/SROA/sroa-addrspace-bug.ll<br>
<br>
diff --git a/lib/Transforms/Scalar/SROA.cpp b/lib/Transforms/Scalar/SROA.cpp<br>
index a0be2c6..3990942 100644<br>
--- a/lib/Transforms/Scalar/SROA.cpp<br>
+++ b/lib/Transforms/Scalar/SROA.cpp<br>
@@ -1416,8 +1416,9 @@ static Value *getAdjustedPtr(IRBuilderTy &IRB, const DataLayout &DL,<br>
<br>
if (!OffsetPtr) {<br>
if (!Int8Ptr) {<br>
- Int8Ptr = IRB.CreateBitCast(Ptr, IRB.getInt8PtrTy(),<br>
- "raw_cast");<br>
+ unsigned PtrAS = Ptr->getType()->getPointerAddressSpace();<br></blockquote><div><br></div><div>Why only pull this out here?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ Int8Ptr = IRB.CreateBitCast(Ptr, IRB.getInt8PtrTy(PtrAS),<br>
+ "raw_cast");<br>
Int8PtrOffset = Offset;<br>
}<br>
<br>
@@ -2485,6 +2486,7 @@ private:<br>
// Strip all inbounds GEPs and pointer casts to try to dig out any root<br>
// alloca that should be re-examined after rewriting this instruction.<br>
Value *OtherPtr = IsDest ? II.getRawSource() : II.getRawDest();<br>
+ unsigned OtherAS = OtherPtr->getType()->getPointerAddressSpace();<br>
if (AllocaInst *AI<br>
= dyn_cast<AllocaInst>(OtherPtr->stripInBoundsOffsets()))<br>
Pass.Worklist.insert(AI);<br>
@@ -2538,6 +2540,9 @@ private:<br>
OtherPtrTy = SubIntTy->getPointerTo();<br>
}<br>
<br>
+ // Fix OtherPtrTy to be in OtherAS address space.<br>
+ OtherPtrTy = OtherPtrTy->getPointerElementType()->getPointerTo(OtherAS);<br>
+<br></blockquote><div><br></div><div>This seems like a bit of a hack. I feel like this isn't really doing a good job of fundamentally thinking about the cases where address spaces are relevant and reasoning about them in a methodical sense... Maybe I'm missing it. I think I have a suggestion below that will help though.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Value *SrcPtr = getAdjustedPtr(IRB, DL, OtherPtr, RelOffset, OtherPtrTy);<br>
Value *DstPtr = &NewAI;<br>
if (!IsDest)<br>
diff --git a/test/Transforms/SROA/sroa-addrspace-bug.ll b/test/Transforms/SROA/sroa-addrspace-bug.ll<br>
new file mode 100644<br>
index 0000000..34e8d82<br>
--- /dev/null<br>
+++ b/test/Transforms/SROA/sroa-addrspace-bug.ll<br>
@@ -0,0 +1,52 @@<br>
+; RUN: opt < %s -sroa -S | FileCheck %s<br>
+<br>
+target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-v16:16:16-v24:32:32-v32:32:32-v48:64:64-v64:64:64-v96:128:128-v128:128:128-v192:256:256-v256:256:256-v512:512:512-v1024:1024:1024-v2048:2048:2048-n32:64"<br>
+target triple = "r600"<br>
+<br>
+%struct.Block = type { i32, i32, i32 }<br>
+<br>
+; CHECK-NOT: bitcast i8 addrspace(1)* {{%[a-zA-z0-9._]+}} to i32*<br></blockquote><div><br></div><div>This is a *really* isolated test case. Have you considered trying to take some of the existing tests and re-write them in ways that are address space sensitive to get more comprehensive coverage?</div>
<div><br></div><div>At the very least, I would like to see specific positive tests combined with a negative test regarding address spaces so it is clear what is supposed to work, and *where* something is supposed to not happen. As-is, this test will provide *no* help to a future maintainer trying to understand why their change regressed it.</div>
</div></div></div>