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

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>

On Thu, Aug 30, 2012 at 2:38 PM, Villmow, Micah <Micah.Villmow at amd.com>
wrote:
> Eli,
>  Here is an updated patch. This is a lot smaller based on your feedback and
still solves the same problem.
The patch appears to be corrupt; can you regenerate it?
> 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.
Suppose I have an array of ten pointers into some address-space which
uses 16-bit pointers, and the default pointer size is 64 bits.  How
many bytes in memory does that take?  To me, it seems like the obvious
answer is 20 bytes, but if you compute it using our current
TargetData, you'll come up with an answer of 80.  That can't work.

If your answer is that it should be 80, and the size of a pointer
isn't something the frontend/IR optimizers should be aware of, I'm not
sure your approach makes sense; you could just introduce custom
load/store nodes in your target which truncate the pointer, and you
wouldn't need to mess with the size of a pointer at all.

-Eli

> -----Original Message-----
> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> Sent: Thursday, August 30, 2012 3:03 PM
> To: Villmow, Micah
> Cc: LLVM Developers Mail
> Subject: Re: [LLVMdev] FW: RFC: Supporting different sized address space
> arithmetic
> 
> On Thu, Aug 30, 2012 at 2:38 PM, Villmow, Micah <Micah.Villmow at
amd.com>
> wrote:
> > Eli,
> >  Here is an updated patch. This is a lot smaller based on your
> feedback and still solves the same problem.
> 
> The patch appears to be corrupt; can you regenerate it?
[Villmow, Micah] Attached.
> 
> > 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.
> 
> Suppose I have an array of ten pointers into some address-space which
> uses 16-bit pointers, and the default pointer size is 64 bits.  How many
> bytes in memory does that take?  To me, it seems like the obvious answer
> is 20 bytes, but if you compute it using our current TargetData, you'll
> come up with an answer of 80.  That can't work.
> 
> If your answer is that it should be 80, and the size of a pointer isn't
> something the frontend/IR optimizers should be aware of, I'm not sure
> your approach makes sense; you could just introduce custom load/store
> nodes in your target which truncate the pointer, and you wouldn't need
> to mess with the size of a pointer at all.
[Villmow, Micah] Yeah I see your point here. I don't deal with array of
pointers in OpenCL, so didn't think of this case.

So, what about extending data layout to support something like:
p#:size:abi:pref <- specify the size, abi and preference for the pointer of
address space '#'. Default behavior is p:size:abi:pref.

I think I could throw this together and get back to you within a week or so, but
need to look into the code more to see how much work it would be.
Doing it this way might actually remove the need to make some of the changes I
made as it becomes a more of a core LLVM issue and less of a codegen
issue.
> 
> -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/8953464b/attachment.obj>