[llvm] d9f5d7b - [InstCombine] Extract bitcast -> gep transform

[David Blaikie via llvm-commits]

On Wed, Jun 23, 2021 at 1:45 PM Nikita Popov <nikita.ppv at gmail.com> wrote:
>
> On Wed, Jun 23, 2021 at 10:18 PM David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Mon, Jun 21, 2021 at 12:25 PM Nikita Popov via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>> >
>> >
>> > Author: Nikita Popov
>> > Date: 2021-06-21T21:24:50+02:00
>> > New Revision: d9f5d7b959de36085944d4a99a73f3053f953796
>> >
>> > URL: https://github.com/llvm/llvm-project/commit/d9f5d7b959de36085944d4a99a73f3053f953796
>> > DIFF: https://github.com/llvm/llvm-project/commit/d9f5d7b959de36085944d4a99a73f3053f953796.diff
>> >
>> > LOG: [InstCombine] Extract bitcast -> gep transform
>> >
>> > Move this into a separate function, to make sure that early
>> > returns do not accidentally skip other transforms. There is
>> > already one isSized() check that could run into this issue,
>> > thus this change is not strictly NFC.
>>
>> Can/should this be tested, then?
>
>
> Yeah :) I ended up reverting this and reapplying with a test in https://github.com/llvm/llvm-project/commit/e2c2124a4b5bad9cf2a1e23a6aef1b2ad753f504.

Oh, awesome, thanks for the pointer!

>>
>> >
>> > Added:
>> >
>> >
>> > Modified:
>> >     llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
>> >
>> > Removed:
>> >
>> >
>> >
>> > ################################################################################
>> > diff  --git a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
>> > index 9b38eb6ab24d..dcfff4552f11 100644
>> > --- a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
>> > +++ b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
>> > @@ -2603,6 +2603,52 @@ Instruction *InstCombinerImpl::optimizeBitCastFromPhi(CastInst &CI,
>> >    return RetVal;
>> >  }
>> >
>> > +static Instruction *convertBitCastToGEP(BitCastInst &CI, IRBuilderBase &Builder,
>> > +                                        const DataLayout &DL) {
>> > +  Value *Src = CI.getOperand(0);
>> > +  PointerType *SrcPTy = cast<PointerType>(Src->getType());
>> > +  PointerType *DstPTy = cast<PointerType>(CI.getType());
>> > +  Type *DstElTy = DstPTy->getElementType();
>> > +  Type *SrcElTy = SrcPTy->getElementType();
>> > +
>> > +  // When the type pointed to is not sized the cast cannot be
>> > +  // turned into a gep.
>> > +  if (!SrcElTy->isSized())
>> > +    return nullptr;
>> > +
>> > +  // If the source and destination are pointers, and this cast is equivalent
>> > +  // to a getelementptr X, 0, 0, 0...  turn it into the appropriate gep.
>> > +  // This can enhance SROA and other transforms that want type-safe pointers.
>> > +  unsigned NumZeros = 0;
>> > +  while (SrcElTy && SrcElTy != DstElTy) {
>> > +    SrcElTy = GetElementPtrInst::getTypeAtIndex(SrcElTy, (uint64_t)0);
>> > +    ++NumZeros;
>> > +  }
>> > +
>> > +  // If we found a path from the src to dest, create the getelementptr now.
>> > +  if (SrcElTy == DstElTy) {
>> > +    SmallVector<Value *, 8> Idxs(NumZeros + 1, Builder.getInt32(0));
>> > +    GetElementPtrInst *GEP =
>> > +        GetElementPtrInst::Create(SrcPTy->getElementType(), Src, Idxs);
>> > +
>> > +    // If the source pointer is dereferenceable, then assume it points to an
>> > +    // allocated object and apply "inbounds" to the GEP.
>> > +    bool CanBeNull, CanBeFreed;
>> > +    if (Src->getPointerDereferenceableBytes(DL, CanBeNull, CanBeFreed)) {
>> > +      // In a non-default address space (not 0), a null pointer can not be
>> > +      // assumed inbounds, so ignore that case (dereferenceable_or_null).
>> > +      // The reason is that 'null' is not treated
>> > diff erently in these address
>> > +      // spaces, and we consequently ignore the 'gep inbounds' special case
>> > +      // for 'null' which allows 'inbounds' on 'null' if the indices are
>> > +      // zeros.
>> > +      if (SrcPTy->getAddressSpace() == 0 || !CanBeNull)
>> > +        GEP->setIsInBounds();
>> > +    }
>> > +    return GEP;
>> > +  }
>> > +  return nullptr;
>> > +}
>> > +
>> >  Instruction *InstCombinerImpl::visitBitCast(BitCastInst &CI) {
>> >    // If the operands are integer typed then apply the integer transforms,
>> >    // otherwise just apply the common ones.
>> > @@ -2616,11 +2662,6 @@ Instruction *InstCombinerImpl::visitBitCast(BitCastInst &CI) {
>> >      return replaceInstUsesWith(CI, Src);
>> >
>> >    if (isa<PointerType>(SrcTy) && isa<PointerType>(DestTy)) {
>> > -    PointerType *SrcPTy = cast<PointerType>(SrcTy);
>> > -    PointerType *DstPTy = cast<PointerType>(DestTy);
>> > -    Type *DstElTy = DstPTy->getElementType();
>> > -    Type *SrcElTy = SrcPTy->getElementType();
>> > -
>> >      // If we are casting a alloca to a pointer to a type of the same
>> >      // size, rewrite the allocation instruction to allocate the "right" type.
>> >      // There is no need to modify malloc calls because it is their bitcast that
>> > @@ -2629,43 +2670,8 @@ Instruction *InstCombinerImpl::visitBitCast(BitCastInst &CI) {
>> >        if (Instruction *V = PromoteCastOfAllocation(CI, *AI))
>> >          return V;
>> >
>> > -    // When the type pointed to is not sized the cast cannot be
>> > -    // turned into a gep.
>> > -    Type *PointeeType =
>> > -        cast<PointerType>(Src->getType()->getScalarType())->getElementType();
>> > -    if (!PointeeType->isSized())
>> > -      return nullptr;
>> > -
>> > -    // If the source and destination are pointers, and this cast is equivalent
>> > -    // to a getelementptr X, 0, 0, 0...  turn it into the appropriate gep.
>> > -    // This can enhance SROA and other transforms that want type-safe pointers.
>> > -    unsigned NumZeros = 0;
>> > -    while (SrcElTy && SrcElTy != DstElTy) {
>> > -      SrcElTy = GetElementPtrInst::getTypeAtIndex(SrcElTy, (uint64_t)0);
>> > -      ++NumZeros;
>> > -    }
>> > -
>> > -    // If we found a path from the src to dest, create the getelementptr now.
>> > -    if (SrcElTy == DstElTy) {
>> > -      SmallVector<Value *, 8> Idxs(NumZeros + 1, Builder.getInt32(0));
>> > -      GetElementPtrInst *GEP =
>> > -          GetElementPtrInst::Create(SrcPTy->getElementType(), Src, Idxs);
>> > -
>> > -      // If the source pointer is dereferenceable, then assume it points to an
>> > -      // allocated object and apply "inbounds" to the GEP.
>> > -      bool CanBeNull, CanBeFreed;
>> > -      if (Src->getPointerDereferenceableBytes(DL, CanBeNull, CanBeFreed)) {
>> > -        // In a non-default address space (not 0), a null pointer can not be
>> > -        // assumed inbounds, so ignore that case (dereferenceable_or_null).
>> > -        // The reason is that 'null' is not treated
>> > diff erently in these address
>> > -        // spaces, and we consequently ignore the 'gep inbounds' special case
>> > -        // for 'null' which allows 'inbounds' on 'null' if the indices are
>> > -        // zeros.
>> > -        if (SrcPTy->getAddressSpace() == 0 || !CanBeNull)
>> > -          GEP->setIsInBounds();
>> > -      }
>> > -      return GEP;
>> > -    }
>> > +    if (Instruction *I = convertBitCastToGEP(CI, Builder, DL))
>> > +      return I;
>> >    }
>> >
>> >    if (FixedVectorType *DestVTy = dyn_cast<FixedVectorType>(DestTy)) {
>> >
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits