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

[Villmow, Micah]

> -----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.
> 
> Ciao, Duncan.
> 
> >>
> >> You seem to think that you can avoid analysing each and every use of
> >> getIntPtrType by doing some tricks inside getIntPtrType.  It just
> >> isn't true, there is no magic bullet.  So please stop trying, you are
> >> just papering over problems.
> >>
> >> Ciao, Duncan.
> >
>