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

[Hal Finkel]

----- 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