PR17925, PATCH

[Nick Lewycky]

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
>>>
>>
>