[llvm-commits] [llvm] r151620 - in /llvm/trunk: lib/Transforms/Scalar/DeadStoreElimination.cpp test/Transforms/DeadStoreElimination/OverwriteStoreStart.ll

[Duncan Sands]

Hi Peter,

> --- llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp Mon Feb 27 22:27:10 2012
> @@ -259,6 +259,13 @@
>     }
>   }
>
> +
> +/// isMemset - Returns true if this instruction is an intrinsic memset
> +static bool isMemset(Instruction *I) {
> +  IntrinsicInst *II = dyn_cast<IntrinsicInst>(I);
> +  return II&&  II->getIntrinsicID() == Intrinsic::memset;
> +}

instead of this you should include IntrinsicInst.h; you can then use
isa<MemSetInst> to test for a memset intrinsic.

> @@ -418,6 +428,21 @@
>         LaterOff<  int64_t(EarlierOff + Earlier.Size)&&
>         int64_t(LaterOff + Later.Size)>= int64_t(EarlierOff + Earlier.Size))
>       return OverwriteEnd;
> +
> +  // The other interesting case is if the later store overwrites the end of
> +  // the earlier store
> +  //
> +  //                    |--earlier--|
> +  //      |--   later   --|
> +  //
> +  // In this case we may want to trim the size of earlier to avoid generating
> +  // writes to addresses which will definitely be overwritten later
> +  if (EarlierOff>= LaterOff&&
> +      EarlierOff<  int64_t(LaterOff + Later.Size)&&
> +      int64_t(EarlierOff + Earlier.Size)>= int64_t(LaterOff + Later.Size)) {

Do you want > in this line?  If there is equality then the memset can be
discarded.

> +    LaterOff = LaterOff + Later.Size;
> +    return OverwriteStart;
> +  }
>
>     // Otherwise, they don't completely overlap.
>     return OverwriteUnknown;
> @@ -589,6 +614,45 @@
>               DepIntrinsic->setLength(TrimmedLength);
>               MadeChange = true;
>             }
> +        } else if (OR == OverwriteStart&&  isMemset(DepWrite)) {
> +          // TODO: base this on the target vector size so that if the earlier
> +          // store was too small to get vector writes anyway then its likely
> +          // a good idea to shorten it
> +          // Power of 2 vector writes are probably always a bad idea to optimize
> +          // as any store/memset/memcpy is likely using vector instructions so
> +          // shortening it to not vector size is likely to be slower
> +          // TODO: shorten memcpy and memmove by offsetting the source address.
> +          MemIntrinsic* DepIntrinsic = cast<MemIntrinsic>(DepWrite);

You could cast to a MemSetInst here.

> +          unsigned DepWriteAlign = DepIntrinsic->getAlignment();
> +          if (llvm::isPowerOf2_64(InstWriteOffset) ||

Why do you accept any power of 2 (eg: 1) here?  If you don't want to reduce
alignment then shouldn't you only do the check below (is DepWriteAlign really
allowed to be zero?  If so, what does it mean?).  If you are happy to reduce
alignment, why do any alignment check at all?

> +              ((DepWriteAlign != 0)&&  InstWriteOffset % DepWriteAlign == 0)) {
> +
> +            DEBUG(dbgs()<<  "DSE: Remove Dead Store:\n  OW START:"
> +<<  *DepWrite<<  "\n  KILLER (offset "
> +<<  InstWriteOffset<<  ", "
> +<<  DepWriteOffset<<  ", "
> +<<  DepLoc.Size<<  ")"
> +<<  *Inst<<  '\n');
> +
> +            Value* DepWriteLength = DepIntrinsic->getLength();
> +            Value* TrimmedLength = ConstantInt::get(DepWriteLength->getType(),
> +                                                    DepLoc.Size -
> +                                                    (InstWriteOffset -
> +                                                    DepWriteOffset));

Should this be DepWriteLength rather than DepLoc.Size?  Are they guaranteed to
be the same?  Also, what happens if the new length is zero?  Finally, is it
guaranteed that this computation doesn't underflow?  If so, maybe you should add
an assert checking that.

> +            DepIntrinsic->setLength(TrimmedLength);
> +            const TargetData *TD = AA->getTargetData();
> +            Type *IntPtrTy = TD->getIntPtrType(BB.getContext());
> +            Value* Offset = ConstantInt::get(IntPtrTy,
> +                                             InstWriteOffset - DepWriteOffset);
> +            // Offset the start of the memset with a GEP.  As the memset type is
> +            // i8* a GEP will do this without needing to use ptrtoint, etc.

I don't think you can rely on it being an i8*, so you should bitcast it to an
i8* first.

> +            Value *Dest = GetElementPtrInst::Create(DepIntrinsic->getRawDest(),
> +                                                    Offset,
> +                                                    "",
> +                                                    DepWrite);
> +            DepIntrinsic->setDest(Dest);
> +            MadeChange = true;
> +          }
>           }
>         }

Ciao, Duncan.