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

[Benjamin Kramer]

On 24.12.2010, at 22:43, Frits van Bommel wrote:

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

Fixed, thanks for pointing that out *smacks forehead*

> 
>> +  // 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)

It should handle all-zero arrays just fine. memcpy/memset/store from undef
shouldn't make it this far.

> 
>> +  // 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.

Agreed, the interface needs some massaging though, currently it completely ignores
alignment, requires but doesn't use TargetData etc.