[PATCH] Check address space when optimizing gep+cast

[Eli Bendersky]

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.

Eli


On Mon, Mar 31, 2014 at 9:59 AM, Jingyue Wu <jingyue at google.com> wrote:

> ping
>
> Matt, does it look good to you?
>
> Jingyue
>
>
> On Fri, Mar 28, 2014 at 3:53 PM, Jingyue Wu <jingyue at google.com> wrote:
>
>> fixed a typo
>>
>>
>> On Fri, Mar 28, 2014 at 3:46 PM, Jingyue Wu <jingyue at google.com> wrote:
>>
>>>
>>>
>>> On Fri, Mar 28, 2014 at 3:05 PM, Eli Bendersky <eliben at google.com>wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Fri, Mar 28, 2014 at 2:25 PM, Jingyue Wu <jingyue at google.com> wrote:
>>>>
>>>>> Hi Matt,
>>>>>
>>>>> That makes sense, and here's a more constructive patch.
>>>>>
>>>>>
>>>> +          if (StrippedPtrTy->getAddressSpace() ==
>>>> GEP.getAddressSpace())
>>>> +            return Res;
>>>> +          // Insert Res, and create an addrspacecast.
>>>> +          return new AddrSpaceCastInst(Builder->Insert(Res),
>>>> GEP.getType());
>>>>
>>>> Can you provide a bit more details in the comment? The surrounding code
>>>> has examples like:
>>>>
>>>> // -> GEP ... stuff
>>>>
>>>> Maybe something like this to clarify the sequence it turns to.
>>>>
>>>
>>> Done
>>>
>>>
>>>>
>>>>
>>>> + at array = internal addrspace(3) global [256 x float] zeroinitializer,
>>>> align 4
>>>> +
>>>> +define float* @keep_necessary_addrspacecast(i64 %i) {
>>>> +entry:
>>>> +; CHECK-LABEL: @keep_necessary_addrspacecast
>>>> +; CHECK: addrspacecast float addrspace(3)* %{{[0-9]+}} to float*
>>>> +  %0 = getelementptr [256 x float]* addrspacecast ([256 x float]
>>>> addrspace(3)* @array to [256 x float]*), i64 0, i64 %i
>>>> +  ret float* %0
>>>> +}
>>>>
>>>>
>>>> Please create test cases that cover both paths in the patch (array /
>>>> not-array).
>>>>
>>>
>>> Done
>>>
>>>
>>>>
>>>> Eli
>>>>
>>>>
>>>>>  Jingyue
>>>>>
>>>>>
>>>>> On Fri, Mar 28, 2014 at 12:07 PM, Matt Arsenault <
>>>>> Matthew.Arsenault at amd.com> wrote:
>>>>>
>>>>>>  On 03/28/2014 12:01 PM, Jingyue Wu wrote:
>>>>>>
>>>>>>  Fixing PR19270. This issue is blocking a waiting patch of mine that
>>>>>> implements the optimization we discussed in
>>>>>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-March/071440.html.
>>>>>>
>>>>>>  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.
>>>>>>
>>>>>>  e.g.,
>>>>>> getelementptr [256 x float]* addrspacecast ([256 x float]
>>>>>> addrspace(3)* @array to [256 x float]*), i64 0, i64 %i
>>>>>> returns a float*, but the optimized instruction
>>>>>> getelementptr [256 x float] addrspace(3)* @array, i64 0, i64 %i
>>>>>>  returns a float addrspace(3)*
>>>>>>
>>>>>> The attached patch disables this optimization when the address space
>>>>>> of the source is different from that of the destination.
>>>>>>
>>>>>>  Jingyue
>>>>>>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> -Matt
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140403/d834e9f4/attachment.html>