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

[Duncan Sands]

Hi Micah,

On 30/10/12 15:15, Villmow, Micah wrote:
>
>
>> -----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.

I've already fixed this in my own tree.  And an important thing to note is that
if you actually read how CastInst::isEliminableCastPair works it is *wrong* to
try to pass the intptr type for DstTy here, in fact for correct operation when
SrcTy and DstTy are pointers (eg ptrtoint + inttoptr case) you have to pass in
the intptr types for both SrcTy *and* DstTy; when MidTy is a pointer (eg: the
inttoptr + ptrtoint case) you have to pass in the intptr type for MidTy.  What
I'm saying is that by just throwing in a random type without carefully analysing
things you are introducing a bug that will result in wrong transforms.

You seem to think that you can avoid analysing each and every use of
getIntPtrType by doing some tricks inside getIntPtrType.  It just isn't true,
there is no magic bullet.  So please stop trying, you are just papering over
problems.

Ciao, Duncan.

> 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
>
>