[LLVMdev] Casting between address spaces and address space semantics

[Matthijs Kooijman]

Hi Mon Ping,

> I don't have a problem having another class, TargetAddrSpace, to store this 
> information.  However, I don't think it make sense being a standalone pass. 
>  Address spaces seems to part of the TargetData and it seems more natural 
> to ask the TargetData to return the TargetAddrSpace object  (similar to 
> struct layout) to describe the relationships between address spaces.
This is pretty much what I did in my first patch, but Chris didn't like it.
Currently, the TargetData class can be completely described using it's string
representation (which is also stored inside a module). Adding address space
information to target data breaks this, unless we also make a string
representation of the address spaces that we can put in a module.

I think that having a seperate class is somewhat cleaner, since it also makes
sure that this info is only made available to the passes that actually use it.

> BTW, there is a comment in TargetAddrspaces.h that indicate the default is
> that all address spaces are equivalent. I assume you meant disjoint here.
Uh, yeah :-)

>> The last part of this patch is an addition to InstCombine to make use of
>> this information: It removes any bitcasts from a subset to a superset
>> address space. It gets at the address space information by requiring the
>> TargetAddrspaces analysis, which will give it the default implementation in
>> all current tools.
>
> For the case of a GetElementPointer, we are replacing a bitcast to a 
> pointer, getelem with a getelem bitcast.  The assumption is the latter 
> bitcast will hopefully go away when we iterate through those uses.
Uh? Is this a comment about what the current code or my patch does, or what it
should do? I don't understand what you mean here.

>> Mon Ping suggests using address space information for alias analysis as
>> well, which seems to make sense. In effect this is a form of type-based
>> alias analysis, but different address spaces don't preclude pointers from
>> being equal. A problem here is that pointers in disjoint address spaces
>> would be marked as not aliasing, but when the default relation is disjoint
>> this is not so conservative. This might require an extra option "Unknown",
>> which can be used as the default instead of "Disjoint". For Unknown, any
>> pass can do the conservative thing.
>
> What I'm suggesting is that Alias Analysis can be a client to where we 
> store address space information.  In the example you gave, alias analysis 
> will examine two memory locations, ask the TargetAddressSpace what the 
> relationship is and if it is disjoint, it will return no alias.  If the 
> address spaces are in subset relationship, the alias analysis returns maybe 
> unless it has more information.  If a client doesn't tell the compiler the 
> correct address space information, the client shouldn't expect correct 
> answers from coming out of the compiler.
True, anyone actually using address space should make sure that this info is
correct anyway. So, no need for an unknown default?

>> Lastly, I'm still not so sure if InstCombine is the right place for this
>> simplification. This needs some more thought, but currently it is a problem
>> that instcombine does not process BitCastConstantExprs. I might end up
>> writing a seperate pass for just this.
>
> I'm not sure either. At some level, what we want is to propagate the most 
> precise address space (or restrict) information to its use.
Exactly.

> This means that ideally we would want to be able to handle copies of the
> value stored in some temporary and track it all the way through to it use.
> InstCombine will not handle this case, e.g, address space 1 is a subset of 2
>    int<1>* ptr = ...
>    int<2>* ptr2 = ptr1+4
>    *ptr2 = ...
Won't this code produce a bitcast in the IR, which can be propagated? My
current patch doesn't do this, but it should be easy to extend it to also
propagate a bitcast past pointer arithmetic.

Ie, it should change

%tmp = bitcast i32 addrspace(1)* %ptr1 to i32 addrspace(2)*
%ptr2 = add i32 addrspace(2)* %tmp, 4
store i32 0 i32 addrspace(2)* %ptr2

to 

%tmp  = add i32 addrspace(1)* %ptr, 4
%ptr2 = bitcast %tmp to i32 addrspace(2)
store i32 0 i32 addrspace(2)* %ptr2

and then to 

%ptr2  = add i32 addrspace(1)* %ptr, 4
store i32 0 i32 addrspace(1)* %ptr2

Which shouldn't be too hard?

Coming to think of it, the above won't even use the add instruction, but will
probably use a GEP to do the +4. The current code already propagates bitcasts
past GEP instructions. Also, I suspect that our C frontends will already
produce the second version of the IR for the C code you give.

Or am I totally missing the point you are making here?


Gr.

Matthijs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080811/59bb448b/attachment.sig>