[LLVMdev] FW: RFC: Supporting different sized address space arithmetic

[Villmow, Micah]

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>