PR17925, PATCH

[Stepan Dyatkovskiy]

Hi Nick,
Thank you for review.
I forbade to treat any pointer as integer, except for those, who belong 
to address space 0.
New patch is attached (tests are included).

-Stepan.

Nick Lewycky wrote:
> Stepan Dyatkovskiy wrote:
>> New patch.
>>
>> In respect to last Nick's remarks:
>> http://llvm.org/bugs/show_bug.cgi?id=17925#c2
>>
>> I made only addrspace(0) equal to integer type by default. I also have
>> added few fixmes, in which I propose to ask target whether we can ignore
>> address space and treat all pointers as integers.
>>
>> I have removed inttoptr-address-space-2.ll, since it would be incorrect
>> after these changes. This test wants to merge function with different
>> return types.
>
> This sounds fine in concept.
>
> Looking at the patch, I would have left out the PtrAsInt stuff for a
> future patch. Right now it's all dead code and we don't add dead code to
> LLVM.
>
> I'm also concerned that the PtrAsInt stuff is wrong. The test is written
> like so:
>
> +  if (TD) {
> +    if (PTy1 && (PTy1->getAddressSpace() == 0 || PtrAsInt))
> +      Ty1 = TD->getIntPtrType(Ty1);
>
> That's not how address spaces work, we'll never have a situation where
> it's safe to say "all address spaces with the same pointer width are the
> same". You may have a situation where address spaces 1, 2, 3 and 4 are
> all i64-sized pointers, but 1 and 2 overlap with each other, and 3 and 4
> overlap with each other, but that's it.
>
> Even if they did overlap, it's not clear to me that an 'inttoptr i64
> 1234 to i64 addrspace(1)*' and 'inttoptr i64 1234 to i64 addrspace(2)*'
> would actually map to the same pointer.
>
> As for address space zero, it's special and magical being the address
> space which both alloca() and malloc()/free() use. There will always be
> an exception for address space zero (put another way, if anybody wants
> to change that they'll have to do an enormous reworking of llvm such
> that changing mergefunc will be the least of their problems).
>
> It looks like LLVM doesn't yet have any way to represent the
> relationship between address spaces, but it's supposed to look similar
> to the same thing from Embedded C. (I forget whether Embedded C permits
> mapping vectors-of-4 to vector-of-3, but LLVM intends to allow this. For
> instance, in addrspace(1) we have memory laid out in '<X,Y,Z>, <X,Y,Z>
> ...' but in addrspace(2) the same memory appears to laid out in
> '<X,Y,Z,1>, <X,Y,Z,1> ...'. This feature is intended for sharing memory
> between CPU and GPU.)
>
> To quote the Embedded C standard, "Typically, the distinction between
> various address spaces is built into the processor's instruction set,
> with accesses to different spaces requiring distinctly different
> instructions." It's important that functions which would use different
> instructions don't get merged by mergefunc.
>
> All that said, this part of the patch is disabled anyways, and
> everything else about this patch looks good!
>
> Nick
>
>>
>> -Stepan
>>
>> Stepan Dyatkovskiy wrote:
>>> ping
>>> Stepan Dyatkovskiy wrote:
>>>> I have updated fix:
>>>> Introduced exceptions for
>>>> * 'ret' instruction (we can ignore address space here)
>>>> * 'load' instruction (we can't ignore address space in operands, but
>>>> it we still can do it for instruction type itself)
>>>>
>>>> Now all tests are passed.
>>>>
>>>> -Stepan
>>>>
>>>> 14.11.2013, 16:38, "Stepan Dyatkovskiy" <stpworld at narod.ru>:
>>>>> Please find bug description at:
>>>>> http://llvm.org/bugs/show_bug.cgi?id=17925
>>>>>
>>>>> Sorry, I forget, that fixes should be published in llvm-commits,
>>>>> not in
>>>>> bugzilla. So, below is fix description.
>>>>>
>>>>> Fix:
>>>>> The are cases when we CAN treat pointers as integers, and places
>>>>> when we
>>>>> CAN'T. Below is my proposal:
>>>>> * We treat pointers as integers when we compare function formal
>>>>> arguments.
>>>>> * Otherwise, we can't do that in general.
>>>>> * We can add more exceptions, where address space affects nothing: for
>>>>> example argument of "ret" instruction.
>>>>> The patch is attached.
>>>>>
>>>>> Though, by now, it breaks 3 tests, but only since these tests checks
>>>>> things that are wrong in general. We have to decide what to do with
>>>>> these things:
>>>>> 1. Can we always ignore address space of 'ret' type?
>>>>> 2. Isn't it imprortant to tread 'load' instruction as different if
>>>>> they
>>>>> accepts pointers with different address spaces?
>>>>>
>>>>> ,
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ptr-int-transitivity-2013-11-26.patch
Type: text/x-diff
Size: 5549 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131126/f7de53c4/attachment.patch>