[llvm-commits] [LLVMdev] Proposal: New IR instruction for casting between address spaces

[Eli Friedman]

On Thu, Sep 20, 2012 at 3:30 PM, Villmow, Micah <Micah.Villmow at amd.com> wrote:
> If I don't bring in TargetData, then there is no way for me to verify the address space size in the verifier or in the auto-upgrade mechanisms.

And that's why I didn't like this approach in the first place.

-Eli

>> -----Original Message-----
>> From: Eli Friedman [mailto:eli.friedman at gmail.com]
>> Sent: Thursday, September 20, 2012 2:32 PM
>> To: Villmow, Micah
>> Cc: Chris Lattner; Mon Ping Wang; llvm-commits at cs.uiuc.edu;
>> llvmdev at cs.uiuc.edu
>> Subject: Re: [LLVMdev] Proposal: New IR instruction for casting between
>> address spaces
>>
>> We can't add a circular dependency between Target and VMCore.
>>
>> -Eli
>>
>> On Thu, Sep 20, 2012 at 8:21 AM, Villmow, Micah <Micah.Villmow at amd.com>
>> wrote:
>> > Ping!
>> >
>> >> -----Original Message-----
>> >> From: Villmow, Micah
>> >> Sent: Tuesday, September 18, 2012 4:12 PM
>> >> To: 'Chris Lattner'; 'Mon Ping Wang'
>> >> Cc: 'llvm-commits at cs.uiuc.edu'; 'llvmdev at cs.uiuc.edu'
>> >> Subject: RE: [LLVMdev] Proposal: New IR instruction for casting
>> >> between address spaces
>> >>
>> >> Resending since I got an error.
>> >>
>> >> > -----Original Message-----
>> >> > From: Villmow, Micah
>> >> > Sent: Tuesday, September 18, 2012 4:04 PM
>> >> > To: Villmow, Micah; 'Chris Lattner'; 'Mon Ping Wang'
>> >> > Cc: 'llvm-commits at cs.uiuc.edu'; 'llvmdev at cs.uiuc.edu'
>> >> > Subject: RE: [LLVMdev] Proposal: New IR instruction for casting
>> >> > between address spaces
>> >> >
>> >> > Added a new patch after some feedback. Also make sure all of the
>> >> > tools/examples build correctly.
>> >> >
>> >> > > -----Original Message-----
>> >> > > From: Villmow, Micah
>> >> > > Sent: Tuesday, September 18, 2012 11:24 AM
>> >> > > To: Villmow, Micah; Chris Lattner; Mon Ping Wang
>> >> > > Cc: llvm-commits at cs.uiuc.edu; llvmdev at cs.uiuc.edu
>> >> > > Subject: RE: [LLVMdev] Proposal: New IR instruction for casting
>> >> > > between address spaces
>> >> > >
>> >> > > Here is the patch that i've developed that implements the below
>> >> > points.
>> >> > > The test itself won't work until the target data changes are
>> added.
>> >> > >
>> >> > > > -----Original Message-----
>> >> > > > From: llvmdev-bounces at cs.uiuc.edu
>> >> > > > [mailto:llvmdev-bounces at cs.uiuc.edu]
>> >> > > > On Behalf Of Villmow, Micah
>> >> > > > Sent: Friday, September 14, 2012 8:16 AM
>> >> > > > To: Chris Lattner; Mon Ping Wang
>> >> > > > Cc: llvmdev at cs.uiuc.edu Mailing List
>> >> > > > Subject: Re: [LLVMdev] Proposal: New IR instruction for casting
>> >> > > > between address spaces
>> >> > > >
>> >> > > >
>> >> > > >
>> >> > > > > -----Original Message-----
>> >> > > > > From: Chris Lattner [mailto:clattner at apple.com]
>> >> > > > > Sent: Thursday, September 13, 2012 11:53 PM
>> >> > > > > To: Mon Ping Wang
>> >> > > > > Cc: Villmow, Micah; llvmdev at cs.uiuc.edu Mailing List
>> >> > > > > Subject: Re: [LLVMdev] Proposal: New IR instruction for
>> >> > > > > casting between address spaces
>> >> > > > >
>> >> > > > >
>> >> > > > > On Sep 13, 2012, at 5:55 PM, Mon Ping Wang
>> >> > > > > <monping at apple.com>
>> >> > > wrote:
>> >> > > > > >>> The pointer size is target dependent so it seems strange
>> >> > > > > >>> to choose
>> >> > > > > an arbitrary size to convert to and from. Are you making a
>> >> > > > > practical argument that 64b is sufficient on all machines so
>> >> > > > > all targets can use that?  In other words, pointers > 64
>> >> > > > > doesn't make any sense in terms of the address space? (A
>> >> > > > > pointer to be >
>> >> > > > > 64 if clients want to use some upper bits to track some state
>> >> > > > > I
>> >> guess).
>> >> > > > > >>>
>> >> > > > > >>> In terms of the three new instructions, one could argue
>> >> > > > > >>> that
>> >> > > > > ptrtoint and intoptr has the same issue or those can also
>> >> > > > > explode in a similar way.  To use them, this seems target
>> >> > > > > dependent so unless we really want to support all the various
>> >> > > > > addressing structures, I rather not have them.
>> >> > > > > >>
>> >> > > > > >> My point is that any producer of this sort of pointer cast
>> >> > > > > >> is
>> >> > > > > already necessarily target specific (it is generating
>> >> > > > > target-specific address space numbers!).  If the front-end
>> >> > > > > knows the address space to use, it can know a safe integer
>> >> > > > > size to
>> >> use.
>> >> > > > > >>
>> >> > > > > >
>> >> > > > > > It depends on what the address space is used for. If I'm
>> >> > > > > > logically
>> >> > > > > partitioning an address space that overlap my pointer size
>> >> > > > > may all be the same size so this issue doesn't come up other
>> >> > > > > than I know the pointer size are the same.
>> >> > > > >
>> >> > > > > Sure, in that case, use bitcast.
>> >> > > > >
>> >> > > > > > My understanding is that is becoming an issue since a
>> >> > > > > > pointer type
>> >> > > > > size could be different for different address space.  I agree
>> >> > > > > for the case where the pointer size is address space
>> >> > > > > dependent that the client has to understand the size and the
>> >> > > > > properties to decide if they need to do truncation, sign
>> >> > > > > extension or zero
>> >> extensions.
>> >> > > > >
>> >> > > > > Right.
>> >> > > > >
>> >> > > > > > This is a problem for auto upgrade as well.  Today, we have
>> >> > > > > > bit cast
>> >> > > > > between same size pointers for different address space.  We
>> >> > > > > would need to do something special for auto upgrade here
>> >> > > > > since the proposal is to not allow bit cast between pointers
>> >> > > > > of different
>> >> > > address spaces.
>> >> > > > >
>> >> > > > > I haven't followed the details of the proposal, but I think
>> >> > > > > it makes perfect sense to continue using bitcast for ptr/ptr
>> >> > > > > casts within the same pointer size.  If you do that, then
>> >> > > > > there is no auto-upgrade
>> >> > > > > issue: all existing bc files can just be assumed to have the
>> >> > > > > same pointer size.
>> >> > > > [Villmow, Micah] So basically we don't need a new IR
>> >> > > > instructions, but instead
>> >> > > > 1) bitcasts between pointers of different size is illegal, the
>> >> > > > proper approach is inttoptr/ptrtoint.
>> >> > > > 2) bitcasts between pointers of the same size stays legal.
>> >> > > > 3) No new IR instruction is needed, as converting between
>> >> > > > pointers of different sizes requires inttoptr/ptrtoint.
>> >> > > >
>> >> > > > The only issues are then to update the verifier to assert on
>> >> > > > bitcasts between pointers of different sizes and add in
>> >> > > > auto-upgrade of binaries to switch to inttoptr/ptrtoint. By
>> >> > > > doing this, I then can clear the way for allowing LLVM to
>> >> > > > support multiple pointer
>> >> > sizes.
>> >> > > >
>> >> > > > Sound good?
>> >> > > >
>> >> > > > Micah
>> >> > > > >
>> >> > > > > -Chris
>> >> > > >
>> >> > > >
>> >> > > >
>> >> > > > _______________________________________________
>> >> > > > LLVM Developers mailing list
>> >> > > > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> >> > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> >
>> >
>> > _______________________________________________
>> > LLVM Developers mailing list
>> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>> >
>
>