[llvm-commits] Patch to add address space support to getIntPtrTy

[Villmow, Micah]

Thanks for the review, so it looks like I should add two new
interfaces, getIntPtrTy(Type*) and getIntPtrTy(MachinePtrInfo*),
 and update the patches accordingly along with the other changes.

Micah

> -----Original Message-----
> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> Sent: Thursday, October 18, 2012 6:31 PM
> To: Villmow, Micah
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] Patch to add address space support to
> getIntPtrTy
> 
> On Thu, Oct 18, 2012 at 4:10 PM, Villmow, Micah <Micah.Villmow at amd.com>
> wrote:
> > Weird, ok, attached the re-updated new one.
> 
> Since you mentioned it, I think getIntPtrTy(Type*) is a very good idea.
> It would simplify a bunch of changes in this patch.  (I would prefer
> Type* over Value* because the meaning is more obvious.)
> 
> Index: lib/Analysis/ScalarEvolution.cpp
> ===================================================================
> --- lib/Analysis/ScalarEvolution.cpp	(revision 166195)
> +++ lib/Analysis/ScalarEvolution.cpp	(working copy)
> @@ -2586,7 +2586,7 @@
>    // constant expression and then folding it back into a ConstantInt.
>    // This is just a compile-time optimization.
>    if (TD)
> -    return getConstant(TD->getIntPtrType(getContext()),
> +    return getConstant(TD->getIntPtrType(getContext(), 0),
>                         TD->getTypeAllocSize(AllocTy));
> 
>    Constant *C = ConstantExpr::getSizeOf(AllocTy);
> 
> I think this needs to take the integer type from the caller.
> 
> @@ -2612,7 +2612,7 @@
>    // constant expression and then folding it back into a ConstantInt.
>    // This is just a compile-time optimization.
>    if (TD)
> -    return getConstant(TD->getIntPtrType(getContext()),
> +    return getConstant(TD->getIntPtrType(getContext(), 0),
>                         TD->getStructLayout(STy)-
> >getElementOffset(FieldNo));
> 
>    Constant *C = ConstantExpr::getOffsetOf(STy, FieldNo);
> 
> Same here.
> 
> Index: lib/CodeGen/SelectionDAG/FastISel.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/FastISel.cpp	(revision 166195)
> +++ lib/CodeGen/SelectionDAG/FastISel.cpp	(working copy)
> @@ -100,10 +100,16 @@
>      return false;
> 
>    // No-op casts are trivially coalesced by fast-isel.
> -  if (const CastInst *Cast = dyn_cast<CastInst>(I))
> -    if (Cast->isNoopCast(TD.getIntPtrType(Cast->getContext())) &&
> +  if (const CastInst *Cast = dyn_cast<CastInst>(I)) {
> +    unsigned AS = 0;
> +    if (Cast->getOpcode() == Instruction::PtrToInt)
> +      AS = Cast->getOperand(0)->getType()->getPointerAddressSpace();
> +    else if (Cast->getOpcode() == Instruction::IntToPtr)
> +      AS = Cast->getType()->getPointerAddressSpace();
> +    if (Cast->isNoopCast(TD.getIntPtrType(Cast->getContext(), AS)) &&
>          !hasTrivialKill(Cast->getOperand(0)))
>        return false;
> +    }
> 
> Can you change isNoopCast to take a DataLayout or something like that?
>  This just makes the code unreadable.
> 
> Index: lib/CodeGen/SelectionDAG/SelectionDAG.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/SelectionDAG.cpp	(revision 166195)
> +++ lib/CodeGen/SelectionDAG/SelectionDAG.cpp	(working copy)
> @@ -3804,7 +3804,8 @@
>    // Emit a library call.
>    TargetLowering::ArgListTy Args;
>    TargetLowering::ArgListEntry Entry;
> -  Entry.Ty = TLI.getDataLayout()->getIntPtrType(*getContext());
> +  unsigned AS = SrcPtrInfo.getAddrSpace();  Entry.Ty =
> + TLI.getDataLayout()->getIntPtrType(*getContext(), AS);
>    Entry.Node = Dst; Args.push_back(Entry);
>    Entry.Node = Src; Args.push_back(Entry);
>    Entry.Node = Size; Args.push_back(Entry);
> 
> This pattern seems common enough that you should just add a
> getIntPtrType somewhere that takes a MachinePointerInfo.
> 
> Index: lib/Transforms/InstCombine/InstCombineCalls.cpp
> ===================================================================
> --- lib/Transforms/InstCombine/InstCombineCalls.cpp	(revision 166195)
> +++ lib/Transforms/InstCombine/InstCombineCalls.cpp	(working copy)
> @@ -996,9 +996,13 @@
>          // Conversion is ok if changing from one pointer type to
> another or from
>          // a pointer to an integer of the same size.
>          !((OldRetTy->isPointerTy() || !TD ||
> -           OldRetTy == TD->getIntPtrType(Caller->getContext())) &&
> +            // FIXME: Not sure what to do here, so setting AS to 0.
> +            // How can the AS for a function call be outside the
> default?
> +           OldRetTy == TD->getIntPtrType(Caller->getContext(), 0)) &&
>            (NewRetTy->isPointerTy() || !TD ||
> -           NewRetTy == TD->getIntPtrType(Caller->getContext()))))
> +            // FIXME: Not sure what to do here, so setting AS to 0.
> +            // How can the AS for a function call be outside the
> default?
> +           NewRetTy == TD->getIntPtrType(Caller->getContext(), 0))))
>        return false;   // Cannot transform this return value.
> 
>      if (!Caller->use_empty() &&
> 
> This code is trying to compare OldRetTy and NewRetTy.  So the relevant
> check is something like "OldRetTy == TD->getIntPtrType(NewRetTy)",
> assuming the new getIntPtrTy(Type*) utility.
> 
> Index: lib/Transforms/InstCombine/InstructionCombining.cpp
> ===================================================================
> --- lib/Transforms/InstCombine/InstructionCombining.cpp	(revision 166195)
> +++ lib/Transforms/InstCombine/InstructionCombining.cpp	(working copy)
> @@ -746,7 +746,8 @@
>    // Start with the index over the outer type.  Note that the type size
>    // might be zero (even if the offset isn't zero) if the indexed type
>    // is something like [0 x {int, int}]
> -  Type *IntPtrTy = TD->getIntPtrType(Ty->getContext());
> +  // FIXME: Not sure what to do here, so setting address space to
> default.
> +  Type *IntPtrTy = TD->getIntPtrType(Ty->getContext(), 0);
>    int64_t FirstIdx = 0;
>    if (int64_t TySize = TD->getTypeAllocSize(Ty)) {
>      FirstIdx = Offset/TySize;
> 
> The integer type needs to come from the caller.
> 
> -Eli