<div dir="ltr">Since this is a bug fix and blocks further changes, and Jingyue addressed the review comments, I went ahead and committed it in r205547. Matt - please let us know if you have any concerns.<div><br></div><div>
Eli</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Mar 31, 2014 at 9:59 AM, Jingyue Wu <span dir="ltr"><<a href="mailto:jingyue@google.com" target="_blank">jingyue@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">ping<div><br></div><div>Matt, does it look good to you? </div><span class="HOEnZb"><font color="#888888"><div>
<br></div><div>Jingyue</div></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Mar 28, 2014 at 3:53 PM, Jingyue Wu <span dir="ltr"><<a href="mailto:jingyue@google.com" target="_blank">jingyue@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">fixed a typo</div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Fri, Mar 28, 2014 at 3:46 PM, Jingyue Wu <span dir="ltr"><<a href="mailto:jingyue@google.com" target="_blank">jingyue@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Fri, Mar 28, 2014 at 3:05 PM, Eli Bendersky <span dir="ltr"><<a href="mailto:eliben@google.com" target="_blank">eliben@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Fri, Mar 28, 2014 at 2:25 PM, Jingyue Wu <span dir="ltr"><<a href="mailto:jingyue@google.com" target="_blank">jingyue@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Matt, <div><br></div><div>That makes sense, and here's a more constructive patch. <span><font color="#888888"><div>
<br></div></font></span></div></div></blockquote><div><br></div></div><div><div>+ if (StrippedPtrTy->getAddressSpace() == GEP.getAddressSpace())</div><div>+ return Res;</div><div>+ // Insert Res, and create an addrspacecast.</div>
<div>+ return new AddrSpaceCastInst(Builder->Insert(Res), GEP.getType());</div></div><div><br></div><div>Can you provide a bit more details in the comment? The surrounding code has examples like:</div><div><br>
</div><div>// -> GEP ... stuff</div><div><br></div><div>Maybe something like this to clarify the sequence it turns to.</div></div></div></div></blockquote><div><br></div></div><div>Done</div><div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div><br></div><div><div>+@array = internal addrspace(3) global [256 x float] zeroinitializer, align 4</div>
<div>+</div><div>+define float* @keep_necessary_addrspacecast(i64 %i) {</div><div>+entry:</div><div>+; CHECK-LABEL: @keep_necessary_addrspacecast</div><div>+; CHECK: addrspacecast float addrspace(3)* %{{[0-9]+}} to float*</div>
<div>+ %0 = getelementptr [256 x float]* addrspacecast ([256 x float] addrspace(3)* @array to [256 x float]*), i64 0, i64 %i</div><div>+ ret float* %0</div><div>+}</div></div><div><br></div><div><br></div><div>Please create test cases that cover both paths in the patch (array / not-array).</div>
</div></div></div></blockquote><div><br></div></div><div>Done</div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote">
<span><font color="#888888">
<div><br></div><div>Eli</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div>
<span><font color="#888888"><div></div><div>Jingyue</div></font></span></div></div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Mar 28, 2014 at 12:07 PM, Matt Arsenault <span dir="ltr"><<a href="mailto:Matthew.Arsenault@amd.com" target="_blank">Matthew.Arsenault@amd.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"><div><div>
<div>On 03/28/2014 12:01 PM, Jingyue Wu
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>Fixing PR19270. This issue is blocking a waiting patch of
mine that implements the optimization we discussed in <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-March/071440.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-March/071440.html</a>. </div>
<div><br>
</div>
visitGetElementPtr in InstructionCombining.cpp gets rid of
unnecessary pointer casts in gep (cast X). However, this
optimization may change the address space of the result pointer
type, and cause type mismatch.
<div>
<br>
</div>
<div>e.g.,</div>
<div>getelementptr [256 x float]* addrspacecast ([256 x float]
addrspace(3)* @array to [256 x float]*), i64 0, i64 %i</div>
<div>returns a float*, but the optimized instruction</div>
<div>getelementptr [256 x float] addrspace(3)* @array, i64 0,
i64 %i<br>
</div>
<div>returns a float addrspace(3)*</div>
<div>
<div>
<div><br>
<div>The attached patch disables this optimization when
the address space of the source is different from that
of the destination.</div>
</div>
</div>
</div>
<div>
<br>
</div>
<div>Jingyue</div>
</div>
</blockquote>
<br></div></div>
If we're doing what I suggested and trying to do operations in the
original address space, then instead of disabling this, why don't
you make this instead just move the addrspacecast to after the GEP?
Do the GEP in addrspace(3), and then addrspacecast that.<span><font color="#888888"><br>
<br>
-Matt<br>
</font></span></div>
</blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>