[llvm-commits] [PATCH] update getPointerTo to handle multiple address spaces

[Duncan Sands]

Hi Micah, hopefully others will chime in on whether it is OK to have
Instructions depend on DataLayout nowadays.  As you point out, isNoopCast
already uses DataLayout but maybe this is a mistake.  In the meantime I've
committed a fix to make isEliminableCastPair work correctly when pointers
have different sizes.  It can be simplified later once a decision is made
on this layering question.

Ciao, Duncan.

On 30/10/12 17:05, Villmow, Micah wrote:
>
>
>> -----Original Message-----
>> From: Duncan Sands [mailto:duncan.sands at gmail.com] On Behalf Of Duncan
>> Sands
>> Sent: Tuesday, October 30, 2012 9:01 AM
>> To: Villmow, Micah
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] [PATCH] update getPointerTo to handle
>> multiple address spaces
>>
>> Hi Micah,
>>
>>>>>>
>>>>>> Once more you are trying to handle the case when the passed in
>>>>>> value is not a pointer.  It's just wrong, please stop trying to do
>> this.
>>>>> [Villmow, Micah] There are places where there is no other way to
>>>> handle this without making the calling code very ugly.
>>>>
>>>> I've already fixed this in my own tree.  And an important thing to
>>>> note is that if you actually read how CastInst::isEliminableCastPair
>>>> works it is *wrong* to try to pass the intptr type for DstTy here, in
>>>> fact for correct operation when SrcTy and DstTy are pointers (eg
>>>> ptrtoint + inttoptr case) you have to pass in the intptr types for
>>>> both SrcTy *and* DstTy; when MidTy is a pointer (eg: the inttoptr +
>>>> ptrtoint case) you have to pass in the intptr type for MidTy.  What
>>>> I'm saying is that by just throwing in a random type without
>>>> carefully analysing things you are introducing a bug that will result
>> in wrong transforms.
>>> [Villmow, Micah] I'm not so sure that you need to pass in both the
>> SrcTy and the DstTy.
>>> I think isEliminableCastPair itself needs to change. IntPtrTy should
>>> not be an argument, but instead the datalayout should be passed in and
>>> the handling of the pointer should occur there. Does this sound like a
>>> better solution than trying to figure out what type is required at the
>> call site?
>>
>> it's just a question of layering.  Is Instructions.cpp allowed to depend
>> on DataLayout?  I think it's clear from the design of
>> isEliminableCastPair that in the past the answer was "no", otherwise why
>> pass in the intptr type rather than DataLayout itself?  With all the
>> recent changes maybe it is OK now, I'm not sure.
> [Villmow, Micah] I agree, in the past it looks like no, but with the movement
> of DataLayout from Target to VMCore, I don't think there is a problem. In fact
> isNoopCast already uses DataLayout, so I think this should be a good change that
> will simplify the callee code and fix my issue with having the callee being
> complex by not getIntPtrTy handling the non-pointer cases.