[PATCH] Canonicalize addrspacecast between different pointer types.

[Michele Scandale]

On 11/18/2013 08:58 PM, Matt Arsenault wrote:
>
> On Nov 18, 2013, at 1:56 AM, Michele Scandale <michele.scandale at gmail.com> wrote:
>
>> On 11/18/2013 03:05 AM, Matt Arsenault wrote:
>>>
>>> On Nov 17, 2013, at 5:12 AM, Michele Scandale <michele.scandale at gmail.com> wrote:
>>>
>>>>
>>>>   I would like to better understand the motivation of this patch: what are the benefit obtained doing a split between the change of address space and the pointer type within the new address space?
>>>>   Being a bitcast just a reintepretation of the input value (no change in the bits) this behavior has been included in the addrspacecast instruction (nobody reported any objection on this).
>>>>
>>>
>>> The addrspacecast instruction’s purpose is to only change the address space. Changing the type of the pointer is a logically separate operation. If separated out, the bitcast could potentially be combined with other instructions that use the value to simplify things. This is similar to how inttoptr / ptrtoint behave. You can use a different size integer than the pointer size, but instcombine splits it into the 2 logically steps of reinterpreting as an integer, and then extending or truncating to the real pointer size.
>>>
>>
>> Originally the /addrspacecast/ was intended to replace every pointer to pointer cast when the two have different address spaces: http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-August/064674.html
>>
>> I'm not convinced too much by the potential benefit of the semantic split you propose: I can understand the case of inttoptr and ptrtoint, but with the addrspacecast we are in a different case.
> It’s probably not the most super-useful thing, but it makes sense.
>
>> Exposing a change of pointer type doesn't seem to me useful: if I need to reinterpret it to a non pointer type an explicit /ptrtoint/ would be required, otherwise an explicit /bitcast/ would be present, but this can be fused in the current /addrspacecast/ itself.
>> Do you have an example that would allow me to understand this?
> Basically anything that visitBitCast could then happen after the addrspacecast. A few more transforms need to be added to look through addrspacecast, but this one will help those cooperate with the existing ones for bitcast.

Yes, but those optimizations are not relevant in this case, because in 
visitBitCast we are looking to the source operand of the bicast and the 
destination type: in this case the source operand would be the addrspacecast. 
Indeed to keep your canonical form you are preventing addrspacecast and bicast 
recombination, this doesn't make sense to me.

>>
>> Probably it would be enough to add the missing optimizations in the InstCombiner.
>>
>
>
>> BTW, if we want to have a strong separation of the change of address space from the rest, I would prefer a radically different solution: a restriction of the current /addrspacecast/ instruction semantic to allow only pointers or vector of pointer with the same base-type.
>>
>
> This would be more annoying to use. I think accepting any types and then canonicalizing it makes sense.
>