llvm dev - Aug 2012 - [LLVMdev] FW: RFC: Supporting different sized address space arithmetic

> -----Original Message-----
> From: Villmow, Micah
> Sent: Friday, August 24, 2012 2:56 PM
> To: 'Eli Friedman'
> Cc: LLVM Developers Mailing List
> Subject: RE: [LLVMdev] RFC: Supporting different sized address space
> arithmetic
> 
> Eli,
>  There is a patch that implements the beginning what I think is the
> correct approach to support the backend overloading the size of the
> pointer for an address space.
> 
> Does this seem like the right way to do this?
> 
> I'm guessing I need to handle a few more nodes(IntToPtr/PtrToInt) and
> atomics, but this already works for what we want to do.
> 
> Micah
> 
> > -----Original Message-----
> > From: Eli Friedman [mailto:eli.friedman at gmail.com]
> > Sent: Friday, August 17, 2012 3:48 PM
> > To: Villmow, Micah
> > Cc: LLVM Developers Mailing List
> > Subject: Re: [LLVMdev] RFC: Supporting different sized address space
> > arithmetic
> >
> > On Fri, Aug 17, 2012 at 3:35 PM, Villmow, Micah
> > <Micah.Villmow at amd.com>
> > wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> > >> Sent: Friday, August 17, 2012 3:16 PM
> > >> To: Villmow, Micah
> > >> Cc: LLVM Developers Mailing List
> > >> Subject: Re: [LLVMdev] RFC: Supporting different sized
address
> > >> space arithmetic
> > >>
> > >> On Fri, Aug 17, 2012 at 2:53 PM, Villmow, Micah
> > >> <Micah.Villmow at amd.com>
> > >> wrote:
> > >> > Currently LLVM only supports address calculations for a
single
> > >> > address space(the default). This is problematic when the
device
> > >> > pointer type is 32/64bits, but there are small distinct
memories
> > >> > that only have 16 bits of addressing(think GPU's
with small
> > >> > software controlled memory
> > >> segments).
> > >> >
> > >> >
> > >> >
> > >> > I am proposing a modification to a few API's in
SelectionDAG that
> > >> > I believe will fix this problem.
> > >> >
> > >> >
> > >> >
> > >> > In TargetLowering
> > >> >
> > >> > *         Add getPointerTy to take an argument, the
address
> space,
> > >> this
> > >> > defaults to 0.
> > >> >
> > >> > *         Add a virtual API call that returns the
default address
> > >> space for
> > >> > the target, this defaults to returning 0.
> > >>
> > >> Under what circumstances would the default address space for
a
> > >> target not be 0?
> > > [Villmow, Micah] In OpenCL, the default address space is the
private
> > > address space. This is closer to TLS in the X86 world, whereas
the
> > > llvm default address space of 0, is closer to the global address
> > > space in OpenCL.  For our GPU's when supporting 64bit
pointers,
> > > there is a huge drop(think ~60%) in LDS bandwidth performance
> > > because of the extra computation 64bit pointers requires. What
would
> > > be ideal is that our default would be treated as global(AS 1) and
be
> > > 64bit pointers,
> > the private(AS 0) would be 32bit pointers and constant(AS 2) and
> > local(AS 3) address spaces can be considered as 16bit pointers. This
> > would allow us to highly optimize our memory access and only pay for
> > the 64bit pointer addressing costs where it is required by hardware.
> > So we want the default address space to be the global address space
> > for all computations the use the pointer type, but on non-OpenCL
> > systems, this behavior might not be warranted.
> >
> > Okay... that mostly makes sense.  (Although it sounds like long-term,
> > you'd want to eliminate all uses of getDefaultAddressSpace from
> > target- independent code.)
> >
> > >> > *         Have the original getPointerTy implementation
with no
> > >> arguments
> > >> > query for the default address space.
> > >> >
> > >> > In SelectionDAG
> > >> >
> > >> > *         Add a new API to getIntPtrConstant that takes
an
> address
> > >> space as
> > >> > the second argument
> > >>
> > >> Do we actually need this API?  Normally, if you need a
> > >> pointer-sized constant, it means you have a value of pointer
type
> > >> somewhere nearby; it's probably easier to just use the
existing API
> > >> which takes a constant and a type than to try to dig the
address
> > >> space out of a memory operation.
> > > [Villmow, Micah] This is only there so that the current
> > > implementation does not change and break anything. Instead of
> > > changing all of the code to the new API, I leave the code alone
and
> > > only change the
> > locations that need the new functionality.
> >
> > I understand that part; it just seems like nobody will actually end up
> > using the two-argument form getIntPtrConstant(), in favor of just
> > using getConstant().
> >
> > >> > *         Modify the implementation of the original
> > getIntPtrConstant
> > >> > function to call the new function with
getDefaultAddressSpace().
> > >> >
> > >> > As far as I can tell, this should not affect any
backends
> > >> > behavior, but will allow the targets with disjoint
address spaces
> > >> > to directly address them in the most efficient manner.
> > >> >
> > >> >
> > >> >
> > >> > So, what do you think? Is this the correct approach? Or
does it
> > >> > actually require more fundamental changes.
> > >>
> > >> This is the right direction; I don't think you need
anything else,
> > >> at least not in the SelectionDAG.
> > >>
> > >> Are you planning on introducing an in-tree target that
actually
> > >> uses this functionality?  I'm afraid it'll bitrot
without good
> testing.
> > > [Villmow, Micah] We have the AMDIL branch that this will be added
> to.
> >
> > That's good.
> >
> > -Eli
-------------- next part --------------
A non-text attachment was scrubbed...
Name: addr_space_variable_pointer.patch
Type: application/octet-stream
Size: 9061 bytes
Desc: addr_space_variable_pointer.patch
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120824/74d5ed54/attachment.obj>

Micah,

There are a number of variable names in this patch that don't follow
the naming convention (which specifies that they should start with an
uppercase letter).
>          if (PtrBits < 64)
> -          OffsVal = DAG.getNode(ISD::TRUNCATE, getCurDebugLoc(),
> -                                TLI.getPointerTy(),
> +          OffsVal = DAG.getNode(ISD::TRUNCATE, getCurDebugLoc(),
> PtrTy, DAG.getConstant(Offs, MVT::i64));
This is not necessarily specific to your patch, but can you explain the
special role of 64-bits here?

Thanks again,
Hal

On Fri, 24 Aug 2012 23:07:54 +0000
"Villmow, Micah" <Micah.Villmow at amd.com> wrote:
> 
> 
> > -----Original Message-----
> > From: Villmow, Micah
> > Sent: Friday, August 24, 2012 2:56 PM
> > To: 'Eli Friedman'
> > Cc: LLVM Developers Mailing List
> > Subject: RE: [LLVMdev] RFC: Supporting different sized address space
> > arithmetic
> > 
> > Eli,
> >  There is a patch that implements the beginning what I think is the
> > correct approach to support the backend overloading the size of the
> > pointer for an address space.
> > 
> > Does this seem like the right way to do this?
> > 
> > I'm guessing I need to handle a few more nodes(IntToPtr/PtrToInt)
> > and atomics, but this already works for what we want to do.
> > 
> > Micah
> > 
> > > -----Original Message-----
> > > From: Eli Friedman [mailto:eli.friedman at gmail.com]
> > > Sent: Friday, August 17, 2012 3:48 PM
> > > To: Villmow, Micah
> > > Cc: LLVM Developers Mailing List
> > > Subject: Re: [LLVMdev] RFC: Supporting different sized address
> > > space arithmetic
> > >
> > > On Fri, Aug 17, 2012 at 3:35 PM, Villmow, Micah
> > > <Micah.Villmow at amd.com>
> > > wrote:
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> > > >> Sent: Friday, August 17, 2012 3:16 PM
> > > >> To: Villmow, Micah
> > > >> Cc: LLVM Developers Mailing List
> > > >> Subject: Re: [LLVMdev] RFC: Supporting different sized
address
> > > >> space arithmetic
> > > >>
> > > >> On Fri, Aug 17, 2012 at 2:53 PM, Villmow, Micah
> > > >> <Micah.Villmow at amd.com>
> > > >> wrote:
> > > >> > Currently LLVM only supports address calculations
for a
> > > >> > single address space(the default). This is
problematic when
> > > >> > the device pointer type is 32/64bits, but there are
small
> > > >> > distinct memories that only have 16 bits of
addressing(think
> > > >> > GPU's with small software controlled memory
> > > >> segments).
> > > >> >
> > > >> >
> > > >> >
> > > >> > I am proposing a modification to a few API's in
SelectionDAG
> > > >> > that I believe will fix this problem.
> > > >> >
> > > >> >
> > > >> >
> > > >> > In TargetLowering
> > > >> >
> > > >> > *         Add getPointerTy to take an argument, the
address
> > space,
> > > >> this
> > > >> > defaults to 0.
> > > >> >
> > > >> > *         Add a virtual API call that returns the
default
> > > >> > address
> > > >> space for
> > > >> > the target, this defaults to returning 0.
> > > >>
> > > >> Under what circumstances would the default address space
for a
> > > >> target not be 0?
> > > > [Villmow, Micah] In OpenCL, the default address space is the
> > > > private address space. This is closer to TLS in the X86
world,
> > > > whereas the llvm default address space of 0, is closer to
the
> > > > global address space in OpenCL.  For our GPU's when
supporting
> > > > 64bit pointers, there is a huge drop(think ~60%) in LDS
> > > > bandwidth performance because of the extra computation 64bit
> > > > pointers requires. What would be ideal is that our default
> > > > would be treated as global(AS 1) and be 64bit pointers,
> > > the private(AS 0) would be 32bit pointers and constant(AS 2) and
> > > local(AS 3) address spaces can be considered as 16bit pointers.
> > > This would allow us to highly optimize our memory access and only
> > > pay for the 64bit pointer addressing costs where it is required
> > > by hardware. So we want the default address space to be the
> > > global address space for all computations the use the pointer
> > > type, but on non-OpenCL systems, this behavior might not be
> > > warranted.
> > >
> > > Okay... that mostly makes sense.  (Although it sounds like
> > > long-term, you'd want to eliminate all uses of
> > > getDefaultAddressSpace from target- independent code.)
> > >
> > > >> > *         Have the original getPointerTy
implementation with
> > > >> > no
> > > >> arguments
> > > >> > query for the default address space.
> > > >> >
> > > >> > In SelectionDAG
> > > >> >
> > > >> > *         Add a new API to getIntPtrConstant that
takes an
> > address
> > > >> space as
> > > >> > the second argument
> > > >>
> > > >> Do we actually need this API?  Normally, if you need a
> > > >> pointer-sized constant, it means you have a value of
pointer
> > > >> type somewhere nearby; it's probably easier to just
use the
> > > >> existing API which takes a constant and a type than to
try to
> > > >> dig the address space out of a memory operation.
> > > > [Villmow, Micah] This is only there so that the current
> > > > implementation does not change and break anything. Instead
of
> > > > changing all of the code to the new API, I leave the code
alone
> > > > and only change the
> > > locations that need the new functionality.
> > >
> > > I understand that part; it just seems like nobody will actually
> > > end up using the two-argument form getIntPtrConstant(), in favor
> > > of just using getConstant().
> > >
> > > >> > *         Modify the implementation of the original
> > > getIntPtrConstant
> > > >> > function to call the new function with
> > > >> > getDefaultAddressSpace().
> > > >> >
> > > >> > As far as I can tell, this should not affect any
backends
> > > >> > behavior, but will allow the targets with disjoint
address
> > > >> > spaces to directly address them in the most
efficient manner.
> > > >> >
> > > >> >
> > > >> >
> > > >> > So, what do you think? Is this the correct
approach? Or does
> > > >> > it actually require more fundamental changes.
> > > >>
> > > >> This is the right direction; I don't think you need
anything
> > > >> else, at least not in the SelectionDAG.
> > > >>
> > > >> Are you planning on introducing an in-tree target that
actually
> > > >> uses this functionality?  I'm afraid it'll
bitrot without good
> > testing.
> > > > [Villmow, Micah] We have the AMDIL branch that this will be
> > > > added
> > to.
> > >
> > > That's good.
> > >
> > > -Eli
> 


-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory

On Fri, Aug 24, 2012 at 4:07 PM, Villmow, Micah <Micah.Villmow at amd.com>
wrote:
>
>
>> -----Original Message-----
>> From: Villmow, Micah
>> Sent: Friday, August 24, 2012 2:56 PM
>> To: 'Eli Friedman'
>> Cc: LLVM Developers Mailing List
>> Subject: RE: [LLVMdev] RFC: Supporting different sized address space
>> arithmetic
>>
>> Eli,
>>  There is a patch that implements the beginning what I think is the
>> correct approach to support the backend overloading the size of the
>> pointer for an address space.
>>
>> Does this seem like the right way to do this?
>>
>> I'm guessing I need to handle a few more nodes(IntToPtr/PtrToInt)
and
>> atomics, but this already works for what we want to do.
Why do we need the change in lib/CodeGen/Analysis.cpp?  If
TargetLowering::getValueType() doesn't work for arbitrary address
spaces, you should fix it.

I think my earlier comments about getIntPtrConstant still hold:
instead of "DAG.getIntPtrConstant(Offset, addrSpace)", you can just
write "DAG.getConstant(Offset, PtrTy)".

+  EVT NewPtrVT = TLI.getPointerTy(dyn_cast<PointerType>(
+        SV->getType())->getAddressSpace());
+  if (PtrVT != NewPtrVT) {
+    // Check to see if we want to change the size of the pointer
+    // based on the address space and if so extend or truncate the pointer.
+    Ptr = DAG.getSExtOrTrunc(Ptr, getCurDebugLoc(), NewPtrVT);
+    PtrVT = NewPtrVT;
+  }

This situation should be impossible: you're checking whether a pointer
has the same type as itself.  If you're hitting this, it's just a
symptom of a bug elsewhere.  The same applies to other places you
perform this sort of check outside of "bitcast" lowering.


The more broad issue I see here is that we need changes to the IR to
make sure the mid-level optimizers don't break your code;
specifically, using "bitcast" for lossy conversions is unsafe (so you
need a "ptrcast" operation which can perform potentially lossy
conversions), and you need to update TargetData so we can correctly
compute the size of an arbitrary pointer.  I was under the impression
that you were planning to propose a fix for those separately, but it
doesn't look like you have.

-Eli

Most likely this code was added before getSExtOrTruncate was added, but not 100%
sure. It seems to assume that no pointer can be more than 64bits in size.
> -----Original Message-----
> From: Hal Finkel [mailto:hfinkel at anl.gov]
> Sent: Friday, August 24, 2012 4:27 PM
> To: Villmow, Micah
> Cc: LLVM Developers Mail
> Subject: Re: [LLVMdev] FW: RFC: Supporting different sized address
> space arithmetic
> 
> Micah,
> 
> There are a number of variable names in this patch that don't follow
> the naming convention (which specifies that they should start with an
> uppercase letter).
> 
> >          if (PtrBits < 64)
> > -          OffsVal = DAG.getNode(ISD::TRUNCATE, getCurDebugLoc(),
> > -                                TLI.getPointerTy(),
> > +          OffsVal = DAG.getNode(ISD::TRUNCATE, getCurDebugLoc(),
> > PtrTy, DAG.getConstant(Offs, MVT::i64));
> 
> This is not necessarily specific to your patch, but can you explain the
> special role of 64-bits here?
> 
> Thanks again,
> Hal
> 
> On Fri, 24 Aug 2012 23:07:54 +0000
> "Villmow, Micah" <Micah.Villmow at amd.com> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: Villmow, Micah
> > > Sent: Friday, August 24, 2012 2:56 PM
> > > To: 'Eli Friedman'
> > > Cc: LLVM Developers Mailing List
> > > Subject: RE: [LLVMdev] RFC: Supporting different sized address
> space
> > > arithmetic
> > >
> > > Eli,
> > >  There is a patch that implements the beginning what I think is
the
> > > correct approach to support the backend overloading the size of
the
> > > pointer for an address space.
> > >
> > > Does this seem like the right way to do this?
> > >
> > > I'm guessing I need to handle a few more
nodes(IntToPtr/PtrToInt)
> > > and atomics, but this already works for what we want to do.
> > >
> > > Micah
> > >
> > > > -----Original Message-----
> > > > From: Eli Friedman [mailto:eli.friedman at gmail.com]
> > > > Sent: Friday, August 17, 2012 3:48 PM
> > > > To: Villmow, Micah
> > > > Cc: LLVM Developers Mailing List
> > > > Subject: Re: [LLVMdev] RFC: Supporting different sized
address
> > > > space arithmetic
> > > >
> > > > On Fri, Aug 17, 2012 at 3:35 PM, Villmow, Micah
> > > > <Micah.Villmow at amd.com>
> > > > wrote:
> > > > >
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Eli Friedman [mailto:eli.friedman at
gmail.com]
> > > > >> Sent: Friday, August 17, 2012 3:16 PM
> > > > >> To: Villmow, Micah
> > > > >> Cc: LLVM Developers Mailing List
> > > > >> Subject: Re: [LLVMdev] RFC: Supporting different
sized address
> > > > >> space arithmetic
> > > > >>
> > > > >> On Fri, Aug 17, 2012 at 2:53 PM, Villmow, Micah
> > > > >> <Micah.Villmow at amd.com>
> > > > >> wrote:
> > > > >> > Currently LLVM only supports address
calculations for a
> > > > >> > single address space(the default). This is
problematic when
> > > > >> > the device pointer type is 32/64bits, but
there are small
> > > > >> > distinct memories that only have 16 bits of
addressing(think
> > > > >> > GPU's with small software controlled
memory
> > > > >> segments).
> > > > >> >
> > > > >> >
> > > > >> >
> > > > >> > I am proposing a modification to a few
API's in SelectionDAG
> > > > >> > that I believe will fix this problem.
> > > > >> >
> > > > >> >
> > > > >> >
> > > > >> > In TargetLowering
> > > > >> >
> > > > >> > *         Add getPointerTy to take an
argument, the address
> > > space,
> > > > >> this
> > > > >> > defaults to 0.
> > > > >> >
> > > > >> > *         Add a virtual API call that returns
the default
> > > > >> > address
> > > > >> space for
> > > > >> > the target, this defaults to returning 0.
> > > > >>
> > > > >> Under what circumstances would the default address
space for a
> > > > >> target not be 0?
> > > > > [Villmow, Micah] In OpenCL, the default address space
is the
> > > > > private address space. This is closer to TLS in the X86
world,
> > > > > whereas the llvm default address space of 0, is closer
to the
> > > > > global address space in OpenCL.  For our GPU's when
supporting
> > > > > 64bit pointers, there is a huge drop(think ~60%) in LDS
> > > > > bandwidth performance because of the extra computation
64bit
> > > > > pointers requires. What would be ideal is that our
default
> > > > > would be treated as global(AS 1) and be 64bit pointers,
> > > > the private(AS 0) would be 32bit pointers and constant(AS 2)
and
> > > > local(AS 3) address spaces can be considered as 16bit
pointers.
> > > > This would allow us to highly optimize our memory access and
only
> > > > pay for the 64bit pointer addressing costs where it is
required
> > > > by hardware. So we want the default address space to be the
> > > > global address space for all computations the use the
pointer
> > > > type, but on non-OpenCL systems, this behavior might not be
> > > > warranted.
> > > >
> > > > Okay... that mostly makes sense.  (Although it sounds like
> > > > long-term, you'd want to eliminate all uses of
> > > > getDefaultAddressSpace from target- independent code.)
> > > >
> > > > >> > *         Have the original getPointerTy
implementation with
> > > > >> > no
> > > > >> arguments
> > > > >> > query for the default address space.
> > > > >> >
> > > > >> > In SelectionDAG
> > > > >> >
> > > > >> > *         Add a new API to getIntPtrConstant
that takes an
> > > address
> > > > >> space as
> > > > >> > the second argument
> > > > >>
> > > > >> Do we actually need this API?  Normally, if you
need a
> > > > >> pointer-sized constant, it means you have a value
of pointer
> > > > >> type somewhere nearby; it's probably easier to
just use the
> > > > >> existing API which takes a constant and a type than
to try to
> > > > >> dig the address space out of a memory operation.
> > > > > [Villmow, Micah] This is only there so that the current
> > > > > implementation does not change and break anything.
Instead of
> > > > > changing all of the code to the new API, I leave the
code alone
> > > > > and only change the
> > > > locations that need the new functionality.
> > > >
> > > > I understand that part; it just seems like nobody will
actually
> > > > end up using the two-argument form getIntPtrConstant(), in
favor
> > > > of just using getConstant().
> > > >
> > > > >> > *         Modify the implementation of the
original
> > > > getIntPtrConstant
> > > > >> > function to call the new function with
> > > > >> > getDefaultAddressSpace().
> > > > >> >
> > > > >> > As far as I can tell, this should not affect
any backends
> > > > >> > behavior, but will allow the targets with
disjoint address
> > > > >> > spaces to directly address them in the most
efficient
> manner.
> > > > >> >
> > > > >> >
> > > > >> >
> > > > >> > So, what do you think? Is this the correct
approach? Or does
> > > > >> > it actually require more fundamental changes.
> > > > >>
> > > > >> This is the right direction; I don't think you
need anything
> > > > >> else, at least not in the SelectionDAG.
> > > > >>
> > > > >> Are you planning on introducing an in-tree target
that
> actually
> > > > >> uses this functionality?  I'm afraid it'll
bitrot without good
> > > testing.
> > > > > [Villmow, Micah] We have the AMDIL branch that this
will be
> > > > > added
> > > to.
> > > >
> > > > That's good.
> > > >
> > > > -Eli
> >
> 
> 
> 
> --
> Hal Finkel
> Postdoctoral Appointee
> Leadership Computing Facility
> Argonne National Laboratory

> -----Original Message-----
> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> Sent: Friday, August 24, 2012 5:37 PM
> To: Villmow, Micah
> Cc: LLVM Developers Mail
> Subject: Re: [LLVMdev] FW: RFC: Supporting different sized address
> space arithmetic
> 
> On Fri, Aug 24, 2012 at 4:07 PM, Villmow, Micah <Micah.Villmow at
amd.com>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Villmow, Micah
> >> Sent: Friday, August 24, 2012 2:56 PM
> >> To: 'Eli Friedman'
> >> Cc: LLVM Developers Mailing List
> >> Subject: RE: [LLVMdev] RFC: Supporting different sized address
space
> >> arithmetic
> >>
> >> Eli,
> >>  There is a patch that implements the beginning what I think is
the
> >> correct approach to support the backend overloading the size of
the
> >> pointer for an address space.
> >>
> >> Does this seem like the right way to do this?
> >>
> >> I'm guessing I need to handle a few more
nodes(IntToPtr/PtrToInt)
> and
> >> atomics, but this already works for what we want to do.
> 
> Why do we need the change in lib/CodeGen/Analysis.cpp?  If
> TargetLowering::getValueType() doesn't work for arbitrary address
> spaces, you should fix it.
[Villmow, Micah] This is what I can figure out fixes it since we are overriding
the address space. Since in this case we are trying to figure out what the
correct value type is, I did not think the normal getValueType should be
overloaded, but if you think it should be, I'll go ahead and make this
change.
> 
> I think my earlier comments about getIntPtrConstant still hold:
> instead of "DAG.getIntPtrConstant(Offset, addrSpace)", you can
just
> write "DAG.getConstant(Offset, PtrTy)".
[Villmow, Micah] I can make the change, just didn't want to make more
changes than were necessary.
> 
> +  EVT NewPtrVT = TLI.getPointerTy(dyn_cast<PointerType>(
> +        SV->getType())->getAddressSpace());
> +  if (PtrVT != NewPtrVT) {
> +    // Check to see if we want to change the size of the pointer
> +    // based on the address space and if so extend or truncate the
> pointer.
> +    Ptr = DAG.getSExtOrTrunc(Ptr, getCurDebugLoc(), NewPtrVT);
> +    PtrVT = NewPtrVT;
> +  }
> 
> This situation should be impossible: you're checking whether a pointer
> has the same type as itself.  If you're hitting this, it's just a
> symptom of a bug elsewhere.  The same applies to other places you
> perform this sort of check outside of "bitcast" lowering.
[Villmow, Micah] Nope, what I'm checking is if the backend has over wrote
the pointer type for this address space. So in the case of 64bit pointers but
local address space, this code gets triggered because the largest pointer size
for the local address space is 16 bits.
> 
> 
> The more broad issue I see here is that we need changes to the IR to
> make sure the mid-level optimizers don't break your code;
> specifically, using "bitcast" for lossy conversions is unsafe (so
you
> need a "ptrcast" operation which can perform potentially lossy
> conversions), and you need to update TargetData so we can correctly
> compute the size of an arbitrary pointer.  I was under the impression
> that you were planning to propose a fix for those separately, but it
> doesn't look like you have.
[Villmow, Micah] I wasn't planning on assuming anything at the optimizer
level would need to be changed. This was only to allow the code generator to
override the pointer size to more correctly match the underlying hardware. If
there is any optimization that this breaks, then the original code was broken
anyways and there is a bug somewhere else. Any valid calculation in a larger
pointer size must also be valid in the smaller pointer size for the address
space to even work since the hardware size is limited by the smaller pointer
size.
> 
> -Eli

Eli,
 Here is an updated patch. This is a lot smaller based on your feedback and
still solves the same problem.

For your comment on the IR changes, I'm reluctant to introduce changes there
because really the backend is overriding the default behavior at a device
specific level. The optimizations themselves can be dangerous, but still should
produce correct results, this only allows the backend to optimize certain cases
and is opt-in.

Micah
> -----Original Message-----
> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> Sent: Friday, August 24, 2012 5:37 PM
> To: Villmow, Micah
> Cc: LLVM Developers Mail
> Subject: Re: [LLVMdev] FW: RFC: Supporting different sized address space
> arithmetic
> 
> On Fri, Aug 24, 2012 at 4:07 PM, Villmow, Micah <Micah.Villmow at
amd.com>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Villmow, Micah
> >> Sent: Friday, August 24, 2012 2:56 PM
> >> To: 'Eli Friedman'
> >> Cc: LLVM Developers Mailing List
> >> Subject: RE: [LLVMdev] RFC: Supporting different sized address
space
> >> arithmetic
> >>
> >> Eli,
> >>  There is a patch that implements the beginning what I think is
the
> >> correct approach to support the backend overloading the size of
the
> >> pointer for an address space.
> >>
> >> Does this seem like the right way to do this?
> >>
> >> I'm guessing I need to handle a few more
nodes(IntToPtr/PtrToInt) and
> >> atomics, but this already works for what we want to do.
> 
> Why do we need the change in lib/CodeGen/Analysis.cpp?  If
> TargetLowering::getValueType() doesn't work for arbitrary address
> spaces, you should fix it.
> 
> I think my earlier comments about getIntPtrConstant still hold:
> instead of "DAG.getIntPtrConstant(Offset, addrSpace)", you can
just
> write "DAG.getConstant(Offset, PtrTy)".
> 
> +  EVT NewPtrVT = TLI.getPointerTy(dyn_cast<PointerType>(
> +        SV->getType())->getAddressSpace());
> +  if (PtrVT != NewPtrVT) {
> +    // Check to see if we want to change the size of the pointer
> +    // based on the address space and if so extend or truncate the
> pointer.
> +    Ptr = DAG.getSExtOrTrunc(Ptr, getCurDebugLoc(), NewPtrVT);
> +    PtrVT = NewPtrVT;
> +  }
> 
> This situation should be impossible: you're checking whether a pointer
> has the same type as itself.  If you're hitting this, it's just a
> symptom of a bug elsewhere.  The same applies to other places you
> perform this sort of check outside of "bitcast" lowering.
> 
> 
> The more broad issue I see here is that we need changes to the IR to
> make sure the mid-level optimizers don't break your code; specifically,
> using "bitcast" for lossy conversions is unsafe (so you need a
"ptrcast"
> operation which can perform potentially lossy conversions), and you need
> to update TargetData so we can correctly compute the size of an
> arbitrary pointer.  I was under the impression that you were planning to
> propose a fix for those separately, but it doesn't look like you have.
> 
> -Eli
-------------- next part --------------
A non-text attachment was scrubbed...
Name: addr_space_variable_pointer.patch
Type: application/octet-stream
Size: 7356 bytes
Desc: addr_space_variable_pointer.patch
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120830/a325781a/attachment.obj>