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

[Peter Cooper]

Hi Duncan

Thanks for all the comments.  I'd been looking for a better way to test an intrinsic being a memset.  I didn't realize isa<> had a check for that.  I'll integrate all your suggestions into round 2 of this work once i've worked out all the buildbot failures.

Cheers,
Pete
On Feb 28, 2012, at 2:35 AM, Duncan Sands <baldrick at free.fr> wrote:

> 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.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits