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

[Villmow, Micah]

So I've been looking into this some more, and once we stick DataLayout here, it requires propogation all throughout the various classes.

For example, all of the constant and constant folding functions that deal with casts now need a data layout propogated through their API so that
it is possible to fold constants in some locations. I think this is overall a good change, but like the data layout change, is quite invasive.

Micah

> -----Original Message-----
> From: Hal Finkel [mailto:hfinkel at anl.gov]
> Sent: Tuesday, October 30, 2012 9:18 AM
> To: Duncan Sands
> Cc: llvm-commits at cs.uiuc.edu; Villmow, Micah
> Subject: Re: [llvm-commits] [PATCH] update getPointerTo to handle
> multiple address spaces
> 
> ----- Original Message -----
> > From: "Duncan Sands" <baldrick at free.fr>
> > To: "Micah Villmow" <Micah.Villmow at amd.com>
> > Cc: llvm-commits at cs.uiuc.edu
> > Sent: Tuesday, October 30, 2012 11:12:01 AM
> > Subject: Re: [llvm-commits] [PATCH] update getPointerTo to handle
> multiple address spaces
> >
> > Hi Micah, hopefully others will chime in on whether it is OK to have
> > Instructions depend on DataLayout nowadays.
> 
> FWIW, I think that now that DataLayout is in VMCore, other components
> in VMCore can depend on it.
> 
>  -Hal
> 
> >  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.
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> 
> --
> Hal Finkel
> Postdoctoral Appointee
> Leadership Computing Facility
> Argonne National Laboratory