[llvm-commits] [PATCH] update getPointerTo to handle multiple address spaces

[Villmow, Micah]

> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Duncan Sands
> Sent: Tuesday, October 30, 2012 7:03 AM
> To: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] update getPointerTo to handle
> multiple address spaces
> 
> Hi Micah,
> 
> On 30/10/12 14:55, Micah Villmow wrote:
> >    This is an updated version of the patch that provides an assertion
> that using certain functions only occurs with certain types.
> 
> I strongly disagree with these changes:
> 
> > @@ -679,10 +676,15 @@
> >  /// least as big as that of a pointer of the given pointer (vector of
> > pointer)  /// type.
> >  Type *DataLayout::getIntPtrType(Type *Ty) const {
> > -  unsigned NumBits = getPointerTypeSizeInBits(Ty);
> > +  unsigned NumBits = 0;
> > +  if ((Ty->isVectorTy() && Ty->getVectorElementType()->isPointerTy())
> ||
> > +      Ty->isPointerTy())
> > +    NumBits = getPointerTypeSizeInBits(Ty);  else
> > +    NumBits = getTypeSizeInBits(Ty);
> >    IntegerType *IntTy = IntegerType::get(Ty->getContext(), NumBits);
> > -  if (VectorType *VecTy = dyn_cast<VectorType>(Ty))
> > -    return VectorType::get(IntTy, VecTy->getNumElements());
> > +  if (Ty->isVectorTy())
> > +    return VectorType::get(IntTy,Ty->getVectorNumElements());
> >    return IntTy;
> >  }
> 
> Once more you are trying to handle the case when the passed in value is
> not a pointer.  It's just wrong, please stop trying to do this.
[Villmow, Micah] There are places where there is no other way to handle this without making the calling code very ugly.
Take InstCombineCasts.cpp around line 243
Instruction::CastOps firstOp = Instruction::CastOps(CI->getOpcode());
Instruction::CastOps secondOp = Instruction::CastOps(opcode);
unsigned Res = CastInst::isEliminableCastPair(firstOp, secondOp, SrcTy, MidTy,
                                                DstTy,
                                  TD ? TD->getIntPtrType(DstTy) : 0);

In some cases, DstTy is a pointer, in other cases it is not. If it is a pointer, then it uses the address space, otherwise it defaults to 0. If I don't allow non-pointer types in these helper functions, I have to either add more conditionals here(and every other location in the code base that this pattern occurs), or I can embed the logic in a function. I prefer the second approach, since its cleaner, and only requires clear documentation on the behavior of the function. 

getIntPtrType as far as I can tell does not require the type be a pointer, it only returns a pointer that corresponds to that type.
getTypeSizeInBits doesn't work for vector of pointers, getPointerTypeSizeInBits only works for pointers or vector of pointers. The reason I added this checks into getIntPtrType was because of r166926, which changed to use getPointerTypeSizeInBits, which asserts in many locations once I added it.
> 
> Ciao, Duncan.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits