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

[Villmow, Micah]

Rest of issues fixed, I'll send a new patch shortly after building/testing.

Micah

> -----Original Message-----
> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> Sent: Friday, October 19, 2012 3:20 PM
> To: Villmow, Micah
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] Patch to add address space support to
> getIntPtrTy
> 
> On Fri, Oct 19, 2012 at 2:57 PM, Villmow, Micah <Micah.Villmow at amd.com>
> wrote:
> > Eli,
> >  I've made the requested modifications and this patch looks a lot
> cleaner with the new API changes.
> 
> +  /// getIntPtrType - Return an unsigned integer type that is the same
> size or
> +  /// greater to the pointer size based on the Type.
> 
> Drop "unsigned"; LLVM IR integers don't have a sign.
> 
> +  // Build a mask for high order bits.
> +  unsigned AS = cast<GEPOperator>(GEP)->getPointerAddressSpace();
>    gep_type_iterator GTI = gep_type_begin(GEP);
> -  Type *IntPtrTy = TD.getIntPtrType(GEP->getContext());
> +  Type *IntPtrTy = TD.getIntPtrType(GEP->getContext(), AS);
>    Value *Result = Constant::getNullValue(IntPtrTy);
> 
> The "build a mask" comment shouldn't move.
> 
> +  // For vector of pointers, we return the size of the address space
> +  // of the pointer type.
> +  if (Ty->isVectorTy() &&
> cast<VectorType>(Ty)->getElementType()->isPointerTy())
> +    return IntegerType::get(C,
> +        getTypeSizeInBits(cast<VectorType>(Ty)->getElementType()));
> 
> If someone called getIntPtrType with a vector, they would probably
> expect a vector result type.
[Villmow, Micah] I think in this case you would use a different API call
if you wanted to get a vector of IntPtrType's. I originally thought the same, but the locations where I hit problems with this pass in a vector of pointers and expect a Integer type in return. This is why I explicitly documented the behavior.
> 
> +  // Otherwise return the address space for the default address space.
> +  return getIntPtrType(C, 0);
> 
> I would prefer an assertion; there isn't any reasonable default here.
[Villmow, Micah] Sure there is, you want to get an IntPtrType in your
code and sometimes you are passed pointers, sometimes vector of pointers
and sometimes vector of integers. So instead of putting conditionals in
the parent code, you just call this helper function.  So by default you
get the default address space, unless your type explicitly calls for
something else. Otherwise we go back to the problem in some locations that
this API was created to avoid.

> Index: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> ===================================================================
> --- lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> 	(revision 166296)
> +++ lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> 	(working copy)
> @@ -173,7 +173,7 @@
>    // Ensure that the alloca array size argument has type intptr_t, so
> that
>    // any casting is exposed early.
>    if (TD) {
> -    Type *IntPtrTy = TD->getIntPtrType(AI.getContext());
> +    Type *IntPtrTy = TD->getIntPtrType(AI.getContext(), 0);
>      if (AI.getArraySize()->getType() != IntPtrTy) {
>        Value *V = Builder->CreateIntCast(AI.getArraySize(),
>                                          IntPtrTy, false);
> 
> Might as well use getIntPtrType(AI.getType()) .
[Villmow, Micah] Well, that requires a little extra processing, so I figured this would just take the quicker API call. But doing this way could allow for AllocaInst to support multiple address spaces in the future, so I'll modify it.
> 
> ===================================================================
> --- lib/Analysis/ConstantFolding.cpp	(revision 166296)
> +++ lib/Analysis/ConstantFolding.cpp	(working copy)
> @@ -377,11 +377,12 @@
>    }
> 
>    if (ConstantExpr *CE = dyn_cast<ConstantExpr>(C)) {
> -    if (CE->getOpcode() == Instruction::IntToPtr &&
> -        CE->getOperand(0)->getType() == TD.getIntPtrType(CE-
> >getContext()))
> +    if (CE->getOpcode() == Instruction::IntToPtr) {
> +      if (CE->getOperand(0)->getType() == TD.getIntPtrType(CE-
> >getType()))
>        return ReadDataFromGlobal(CE->getOperand(0), ByteOffset, CurPtr,
>                                  BytesLeft, TD);
>    }
> +  }
> 
>    // Otherwise, unknown initializer type.
>    return false;
> 
> Why split the if statement?
[Villmow, Micah] Legacy change, fixed.