PR17925, PATCH

[Stepan Dyatkovskiy]

r195769.
-Stepan.

Nick Lewycky wrote:
> Stepan Dyatkovskiy wrote:
>> 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).
>
> LGTM. Thanks!!
>
>>
>> -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
>>>>
>>>
>>
>