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

[Villmow, 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.
[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