PR17925, PATCH

[Nick Lewycky]

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
>