[llvm-commits] [llvm] r122548 - in /llvm/trunk: lib/Transforms/Scalar/MemCpyOptimizer.cpp test/Transforms/MemCpyOpt/memcpy-to-memset.ll

[Frits van Bommel]

On Fri, Dec 24, 2010 at 10:17 PM, Benjamin Kramer
<benny.kra at googlemail.com> wrote:
>  /// isBytewiseValue - If the specified value can be set by repeating the same
>  /// byte in memory, return the i8 value that it is represented with.  This is
> @@ -38,6 +40,13 @@
>  /// i16 0xF0F0, double 0.0 etc.  If the value can't be handled with a repeated
>  /// byte store (e.g. i16 0x1234), return null.
>  static Value *isBytewiseValue(Value *V) {
> +  // Look through constant globals.
> +  if (GlobalVariable *GV = dyn_cast<GlobalVariable>(V)) {
> +    if (GV->mayBeOverridden() || !GV->isConstant() || !GV->hasInitializer())
> +      return 0;
> +    V = GV->getInitializer();
> +  }
> +

This shouldn't be done here. Putting this here can cause
miscompilation when a pointer to a global is stored somewhere (by
memset'ing the bytewise value of the *initializer* instead).
This should probably be put into a separate function (e.g.
isPointerToBytewiseValue()) that returns
isBytewiseValue(GV->getInitializer()) if the conditions hold.

> +  // A ConstantArray is splatable if all its members are equal and also
> +  // splatable.
> +  if (ConstantArray *CA = dyn_cast<ConstantArray>(V)) {

I notice this function doesn't seem to handle null values. There
should probably be an
  if (V->isNullValue()) return Zero;
or similar near here.
Possibly even the same for undefs.
(Currently it doesn't handle all-zero arrays & structs, for instance)

> +  // If copying from a constant, try to turn the memcpy into a memset.
> +  if (Value *ByteVal = isBytewiseValue(M->getSource())) {

This should use the above-mentioned isPointerToBytewiseValue(...).

> +    Value *Ops[] = {
> +      M->getRawDest(), ByteVal,               // Start, value
> +      CopySize,                               // Size
> +      M->getAlignmentCst(),                   // Alignment
> +      ConstantInt::getFalse(M->getContext()), // volatile
> +    };
> +    const Type *Tys[] = { Ops[0]->getType(), Ops[2]->getType() };
> +    Module *Mod = M->getParent()->getParent()->getParent();
> +    Function *MemSetF = Intrinsic::getDeclaration(Mod, Intrinsic::memset, Tys, 2);
> +    CallInst::Create(MemSetF, Ops, Ops+5, "", M);

This can probably be simplified by using EmitMemSet() from
llvm/Transforms/Utils/BuildLibCalls.h, but the same goes for the
previously existing code in this file.