[llvm] f79e6a8 - [MemCpyOptimizer] Simplify API of processStore and processMem* functions
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 12 14:08:08 PDT 2020
After speaking with Jay offline they've gone ahead and said to revert while
we get a testcase.
Reverted thusly:
commit b422fe7d626b878b4645e3e0449f16cddaa5eb5c
Author: Eric Christopher <echristo at gmail.com>
Date: Fri Jun 12 14:01:27 2020
Temporarily revert "[MemCpyOptimizer] Simplify API of processStore and
processMem* functions"
as it seems to be causing some internal crashes in AA after
email with the author.
This reverts commit f79e6a8847aa330cac6837168d02f6b319024858.
On Fri, Jun 12, 2020 at 1:34 PM Eric Christopher <echristo at gmail.com> wrote:
> FWIW I'm starting to see some crashes in the compiler that are root
> causing to this patch. Currently trying to get a testcase and see what's
> happening, but letting you know now and we might need to revert this.
>
> Thanks!
>
> -eric
>
> On Thu, Jun 11, 2020 at 4:48 AM Jay Foad via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>>
>> Author: Jay Foad
>> Date: 2020-06-11T12:48:09+01:00
>> New Revision: f79e6a8847aa330cac6837168d02f6b319024858
>>
>> URL:
>> https://github.com/llvm/llvm-project/commit/f79e6a8847aa330cac6837168d02f6b319024858
>> DIFF:
>> https://github.com/llvm/llvm-project/commit/f79e6a8847aa330cac6837168d02f6b319024858.diff
>>
>> LOG: [MemCpyOptimizer] Simplify API of processStore and processMem*
>> functions
>>
>> Previously these functions either returned a "changed" flag or a "repeat
>> instruction" flag, and could also modify an iterator to control which
>> instruction would be processed next.
>>
>> Simplify this by always returning a "changed" flag, and handling all of
>> the "repeat instruction" functionality by modifying the iterator.
>>
>> No functional change intended except in this case:
>> // If the source and destination of the memcpy are the same, then zap it.
>> ... where the previous code failed to process the instruction after the
>> zapped memcpy.
>>
>> Differential Revision: https://reviews.llvm.org/D81540
>>
>> Added:
>>
>>
>> Modified:
>> llvm/include/llvm/Transforms/Scalar/MemCpyOptimizer.h
>> llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>>
>> Removed:
>>
>>
>>
>>
>> ################################################################################
>> diff --git a/llvm/include/llvm/Transforms/Scalar/MemCpyOptimizer.h
>> b/llvm/include/llvm/Transforms/Scalar/MemCpyOptimizer.h
>> index 1570c3a08d0b..a4e2012041d8 100644
>> --- a/llvm/include/llvm/Transforms/Scalar/MemCpyOptimizer.h
>> +++ b/llvm/include/llvm/Transforms/Scalar/MemCpyOptimizer.h
>> @@ -59,12 +59,12 @@ class MemCpyOptPass : public
>> PassInfoMixin<MemCpyOptPass> {
>> bool processStore(StoreInst *SI, BasicBlock::iterator &BBI);
>> bool processMemSet(MemSetInst *SI, BasicBlock::iterator &BBI);
>> bool processMemCpy(MemCpyInst *M, BasicBlock::iterator &BBI);
>> - bool processMemMove(MemMoveInst *M);
>> + bool processMemMove(MemMoveInst *M, BasicBlock::iterator &BBI);
>> bool performCallSlotOptzn(Instruction *cpy, Value *cpyDst, Value
>> *cpySrc,
>> uint64_t cpyLen, Align cpyAlign, CallInst
>> *C);
>> - bool processMemCpyMemCpyDependence(MemCpyInst *M, MemCpyInst *MDep);
>> - bool processMemSetMemCpyDependence(MemCpyInst *M, MemSetInst *MDep);
>> - bool performMemCpyToMemSetOptzn(MemCpyInst *M, MemSetInst *MDep);
>> + Instruction *processMemCpyMemCpyDependence(MemCpyInst *M, MemCpyInst
>> *MDep);
>> + Instruction *processMemSetMemCpyDependence(MemCpyInst *M, MemSetInst
>> *MDep);
>> + Instruction *performMemCpyToMemSetOptzn(MemCpyInst *M, MemSetInst
>> *MDep);
>> bool processByValArgument(CallBase &CB, unsigned ArgNo);
>> Instruction *tryMergingIntoMemset(Instruction *I, Value *StartPtr,
>> Value *ByteVal);
>>
>> diff --git a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>> b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>> index 4b4196edc12b..69881d6155db 100644
>> --- a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>> +++ b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>> @@ -501,6 +501,8 @@ static bool moveUp(AliasAnalysis &AA, StoreInst *SI,
>> Instruction *P,
>> return true;
>> }
>>
>> +/// If changes are made, return true and set BBI to the next instruction
>> to
>> +/// visit.
>> bool MemCpyOptPass::processStore(StoreInst *SI, BasicBlock::iterator
>> &BBI) {
>> if (!SI->isSimple()) return false;
>>
>> @@ -578,7 +580,6 @@ bool MemCpyOptPass::processStore(StoreInst *SI,
>> BasicBlock::iterator &BBI) {
>> LI->eraseFromParent();
>> ++NumMemCpyInstr;
>>
>> - // Make sure we do not invalidate the iterator.
>> BBI = M->getIterator();
>> return true;
>> }
>> @@ -642,7 +643,7 @@ bool MemCpyOptPass::processStore(StoreInst *SI,
>> BasicBlock::iterator &BBI) {
>> if (Value *ByteVal = isBytewiseValue(V, DL)) {
>> if (Instruction *I = tryMergingIntoMemset(SI,
>> SI->getPointerOperand(),
>> ByteVal)) {
>> - BBI = I->getIterator(); // Don't invalidate iterator.
>> + BBI = I->getIterator();
>> return true;
>> }
>>
>> @@ -662,7 +663,6 @@ bool MemCpyOptPass::processStore(StoreInst *SI,
>> BasicBlock::iterator &BBI) {
>> SI->eraseFromParent();
>> NumMemSetInfer++;
>>
>> - // Make sure we do not invalidate the iterator.
>> BBI = M->getIterator();
>> return true;
>> }
>> @@ -671,13 +671,15 @@ bool MemCpyOptPass::processStore(StoreInst *SI,
>> BasicBlock::iterator &BBI) {
>> return false;
>> }
>>
>> +/// If changes are made, return true and set BBI to the next instruction
>> to
>> +/// visit.
>> bool MemCpyOptPass::processMemSet(MemSetInst *MSI, BasicBlock::iterator
>> &BBI) {
>> // See if there is another memset or store neighboring this memset
>> which
>> // allows us to widen out the memset to do a single larger store.
>> if (isa<ConstantInt>(MSI->getLength()) && !MSI->isVolatile())
>> if (Instruction *I = tryMergingIntoMemset(MSI, MSI->getDest(),
>> MSI->getValue())) {
>> - BBI = I->getIterator(); // Don't invalidate iterator.
>> + BBI = I->getIterator();
>> return true;
>> }
>> return false;
>> @@ -886,12 +888,12 @@ bool
>> MemCpyOptPass::performCallSlotOptzn(Instruction *cpy, Value *cpyDest,
>>
>> /// We've found that the (upward scanning) memory dependence of memcpy
>> 'M' is
>> /// the memcpy 'MDep'. Try to simplify M to copy from MDep's input if we
>> can.
>> -bool MemCpyOptPass::processMemCpyMemCpyDependence(MemCpyInst *M,
>> - MemCpyInst *MDep) {
>> +Instruction *MemCpyOptPass::processMemCpyMemCpyDependence(MemCpyInst *M,
>> + MemCpyInst
>> *MDep) {
>> // We can only transforms memcpy's where the dest of one is the source
>> of the
>> // other.
>> if (M->getSource() != MDep->getDest() || MDep->isVolatile())
>> - return false;
>> + return nullptr;
>>
>> // If dep instruction is reading from our current input, then it is a
>> noop
>> // transfer and substituting the input won't change this instruction.
>> Just
>> @@ -899,14 +901,14 @@ bool
>> MemCpyOptPass::processMemCpyMemCpyDependence(MemCpyInst *M,
>> // memcpy(a <- a)
>> // memcpy(b <- a)
>> if (M->getSource() == MDep->getSource())
>> - return false;
>> + return nullptr;
>>
>> // Second, the length of the memcpy's must be the same, or the
>> preceding one
>> // must be larger than the following one.
>> ConstantInt *MDepLen = dyn_cast<ConstantInt>(MDep->getLength());
>> ConstantInt *MLen = dyn_cast<ConstantInt>(M->getLength());
>> if (!MDepLen || !MLen || MDepLen->getZExtValue() <
>> MLen->getZExtValue())
>> - return false;
>> + return nullptr;
>>
>> AliasAnalysis &AA = LookupAliasAnalysis();
>>
>> @@ -926,7 +928,7 @@ bool
>> MemCpyOptPass::processMemCpyMemCpyDependence(MemCpyInst *M,
>> MD->getPointerDependencyFrom(MemoryLocation::getForSource(MDep),
>> false,
>> M->getIterator(), M->getParent());
>> if (!SourceDep.isClobber() || SourceDep.getInst() != MDep)
>> - return false;
>> + return nullptr;
>>
>> // If the dest of the second might alias the source of the first, then
>> the
>> // source and dest might overlap. We still want to eliminate the
>> intermediate
>> @@ -943,20 +945,21 @@ bool
>> MemCpyOptPass::processMemCpyMemCpyDependence(MemCpyInst *M,
>> // TODO: Is this worth it if we're creating a less aligned memcpy? For
>> // example we could be moving from movaps -> movq on x86.
>> IRBuilder<> Builder(M);
>> + Instruction *MC;
>> if (UseMemMove)
>> - Builder.CreateMemMove(M->getRawDest(), M->getDestAlign(),
>> - MDep->getRawSource(), MDep->getSourceAlign(),
>> - M->getLength(), M->isVolatile());
>> + MC = Builder.CreateMemMove(M->getRawDest(), M->getDestAlign(),
>> + MDep->getRawSource(),
>> MDep->getSourceAlign(),
>> + M->getLength(), M->isVolatile());
>> else
>> - Builder.CreateMemCpy(M->getRawDest(), M->getDestAlign(),
>> - MDep->getRawSource(), MDep->getSourceAlign(),
>> - M->getLength(), M->isVolatile());
>> + MC = Builder.CreateMemCpy(M->getRawDest(), M->getDestAlign(),
>> + MDep->getRawSource(),
>> MDep->getSourceAlign(),
>> + M->getLength(), M->isVolatile());
>>
>> // Remove the instruction we're replacing.
>> MD->removeInstruction(M);
>> M->eraseFromParent();
>> ++NumMemCpyInstr;
>> - return true;
>> + return MC;
>> }
>>
>> /// We've found that the (upward scanning) memory dependence of \p
>> MemCpy is
>> @@ -973,18 +976,18 @@ bool
>> MemCpyOptPass::processMemCpyMemCpyDependence(MemCpyInst *M,
>> /// memcpy(dst, src, src_size);
>> /// memset(dst + src_size, c, dst_size <= src_size ? 0 : dst_size -
>> src_size);
>> /// \endcode
>> -bool MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst *MemCpy,
>> - MemSetInst *MemSet) {
>> +Instruction *MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst
>> *MemCpy,
>> + MemSetInst
>> *MemSet) {
>> // We can only transform memset/memcpy with the same destination.
>> if (MemSet->getDest() != MemCpy->getDest())
>> - return false;
>> + return nullptr;
>>
>> // Check that there are no other dependencies on the memset
>> destination.
>> MemDepResult DstDepInfo =
>> MD->getPointerDependencyFrom(MemoryLocation::getForDest(MemSet),
>> false,
>> MemCpy->getIterator(),
>> MemCpy->getParent());
>> if (DstDepInfo.getInst() != MemSet)
>> - return false;
>> + return nullptr;
>>
>> // Use the same i8* dest as the memcpy, killing the memset dest if
>> diff erent.
>> Value *Dest = MemCpy->getRawDest();
>> @@ -1016,14 +1019,14 @@ bool
>> MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst *MemCpy,
>> Value *SizeDiff = Builder.CreateSub(DestSize, SrcSize);
>> Value *MemsetLen = Builder.CreateSelect(
>> Ule, ConstantInt::getNullValue(DestSize->getType()), SizeDiff);
>> - Builder.CreateMemSet(
>> + auto *MS = Builder.CreateMemSet(
>> Builder.CreateGEP(Dest->getType()->getPointerElementType(), Dest,
>> SrcSize),
>> MemSet->getOperand(1), MemsetLen, MaybeAlign(Align));
>>
>> MD->removeInstruction(MemSet);
>> MemSet->eraseFromParent();
>> - return true;
>> + return MS;
>> }
>>
>> /// Determine whether the instruction has undefined content for the
>> given Size,
>> @@ -1055,19 +1058,19 @@ static bool hasUndefContents(Instruction *I,
>> ConstantInt *Size) {
>> /// When dst2_size <= dst1_size.
>> ///
>> /// The \p MemCpy must have a Constant length.
>> -bool MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst *MemCpy,
>> - MemSetInst *MemSet) {
>> +Instruction *MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst
>> *MemCpy,
>> + MemSetInst
>> *MemSet) {
>> AliasAnalysis &AA = LookupAliasAnalysis();
>>
>> // Make sure that memcpy(..., memset(...), ...), that is we are
>> memsetting and
>> // memcpying from the same address. Otherwise it is hard to reason
>> about.
>> if (!AA.isMustAlias(MemSet->getRawDest(), MemCpy->getRawSource()))
>> - return false;
>> + return nullptr;
>>
>> // A known memset size is required.
>> ConstantInt *MemSetSize = dyn_cast<ConstantInt>(MemSet->getLength());
>> if (!MemSetSize)
>> - return false;
>> + return nullptr;
>>
>> // Make sure the memcpy doesn't read any more than what the memset
>> wrote.
>> // Don't worry about sizes larger than i64.
>> @@ -1083,13 +1086,12 @@ bool
>> MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst *MemCpy,
>> if (DepInfo.isDef() && hasUndefContents(DepInfo.getInst(), CopySize))
>> CopySize = MemSetSize;
>> else
>> - return false;
>> + return nullptr;
>> }
>>
>> IRBuilder<> Builder(MemCpy);
>> - Builder.CreateMemSet(MemCpy->getRawDest(), MemSet->getOperand(1),
>> CopySize,
>> - MaybeAlign(MemCpy->getDestAlignment()));
>> - return true;
>> + return Builder.CreateMemSet(MemCpy->getRawDest(),
>> MemSet->getOperand(1),
>> + CopySize,
>> MaybeAlign(MemCpy->getDestAlignment()));
>> }
>>
>> /// Perform simplification of memcpy's. If we have memcpy A
>> @@ -1097,40 +1099,49 @@ bool
>> MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst *MemCpy,
>> /// B to be a memcpy from X to Z (or potentially a memmove, depending on
>> /// circumstances). This allows later passes to remove the first memcpy
>> /// altogether.
>> +/// If changes are made, return true and set BBI to the next instruction
>> to
>> +/// visit.
>> bool MemCpyOptPass::processMemCpy(MemCpyInst *M, BasicBlock::iterator
>> &BBI) {
>> // We can only optimize non-volatile memcpy's.
>> if (M->isVolatile()) return false;
>>
>> // If the source and destination of the memcpy are the same, then zap
>> it.
>> if (M->getSource() == M->getDest()) {
>> - ++BBI;
>> MD->removeInstruction(M);
>> M->eraseFromParent();
>> return true;
>> }
>>
>> // If copying from a constant, try to turn the memcpy into a memset.
>> - if (GlobalVariable *GV = dyn_cast<GlobalVariable>(M->getSource()))
>> - if (GV->isConstant() && GV->hasDefinitiveInitializer())
>> + if (GlobalVariable *GV = dyn_cast<GlobalVariable>(M->getSource())) {
>> + if (GV->isConstant() && GV->hasDefinitiveInitializer()) {
>> if (Value *ByteVal = isBytewiseValue(GV->getInitializer(),
>>
>> M->getModule()->getDataLayout())) {
>> IRBuilder<> Builder(M);
>> - Builder.CreateMemSet(M->getRawDest(), ByteVal, M->getLength(),
>> - MaybeAlign(M->getDestAlignment()), false);
>> + auto *MS =
>> + Builder.CreateMemSet(M->getRawDest(), ByteVal,
>> M->getLength(),
>> + MaybeAlign(M->getDestAlignment()),
>> false);
>> MD->removeInstruction(M);
>> M->eraseFromParent();
>> ++NumCpyToSet;
>> + BBI = MS->getIterator();
>> return true;
>> }
>> + }
>> + }
>>
>> MemDepResult DepInfo = MD->getDependency(M);
>>
>> // Try to turn a partially redundant memset + memcpy into
>> // memcpy + smaller memset. We don't need the memcpy size for this.
>> - if (DepInfo.isClobber())
>> - if (MemSetInst *MDep = dyn_cast<MemSetInst>(DepInfo.getInst()))
>> - if (processMemSetMemCpyDependence(M, MDep))
>> + if (DepInfo.isClobber()) {
>> + if (MemSetInst *MDep = dyn_cast<MemSetInst>(DepInfo.getInst())) {
>> + if (auto *MS = processMemSetMemCpyDependence(M, MDep)) {
>> + BBI = MS->getIterator();
>> return true;
>> + }
>> + }
>> + }
>>
>> // The optimizations after this point require the memcpy size.
>> ConstantInt *CopySize = dyn_cast<ConstantInt>(M->getLength());
>> @@ -1163,8 +1174,13 @@ bool MemCpyOptPass::processMemCpy(MemCpyInst *M,
>> BasicBlock::iterator &BBI) {
>> SrcLoc, true, M->getIterator(), M->getParent());
>>
>> if (SrcDepInfo.isClobber()) {
>> - if (MemCpyInst *MDep = dyn_cast<MemCpyInst>(SrcDepInfo.getInst()))
>> - return processMemCpyMemCpyDependence(M, MDep);
>> + if (MemCpyInst *MDep = dyn_cast<MemCpyInst>(SrcDepInfo.getInst())) {
>> + if (auto *MC = processMemCpyMemCpyDependence(M, MDep)) {
>> + BBI = MC->getIterator();
>> + return true;
>> + }
>> + return false;
>> + }
>> } else if (SrcDepInfo.isDef()) {
>> if (hasUndefContents(SrcDepInfo.getInst(), CopySize)) {
>> MD->removeInstruction(M);
>> @@ -1176,10 +1192,11 @@ bool MemCpyOptPass::processMemCpy(MemCpyInst *M,
>> BasicBlock::iterator &BBI) {
>>
>> if (SrcDepInfo.isClobber())
>> if (MemSetInst *MDep = dyn_cast<MemSetInst>(SrcDepInfo.getInst()))
>> - if (performMemCpyToMemSetOptzn(M, MDep)) {
>> + if (auto *MS = performMemCpyToMemSetOptzn(M, MDep)) {
>> MD->removeInstruction(M);
>> M->eraseFromParent();
>> ++NumCpyToSet;
>> + BBI = MS->getIterator();
>> return true;
>> }
>>
>> @@ -1188,7 +1205,9 @@ bool MemCpyOptPass::processMemCpy(MemCpyInst *M,
>> BasicBlock::iterator &BBI) {
>>
>> /// Transforms memmove calls to memcpy calls when the src/dst are
>> guaranteed
>> /// not to alias.
>> -bool MemCpyOptPass::processMemMove(MemMoveInst *M) {
>> +/// If changes are made, return true and set BBI to the next instruction
>> to
>> +/// visit.
>> +bool MemCpyOptPass::processMemMove(MemMoveInst *M, BasicBlock::iterator
>> &BBI) {
>> AliasAnalysis &AA = LookupAliasAnalysis();
>>
>> if (!TLI->has(LibFunc_memmove))
>> @@ -1214,6 +1233,7 @@ bool MemCpyOptPass::processMemMove(MemMoveInst *M) {
>> MD->removeInstruction(M);
>>
>> ++NumMoveToCpy;
>> + BBI = M->getIterator();
>> return true;
>> }
>>
>> @@ -1316,28 +1336,19 @@ bool MemCpyOptPass::iterateOnFunction(Function
>> &F) {
>> // Avoid invalidating the iterator.
>> Instruction *I = &*BI++;
>>
>> - bool RepeatInstruction = false;
>> -
>> if (StoreInst *SI = dyn_cast<StoreInst>(I))
>> MadeChange |= processStore(SI, BI);
>> else if (MemSetInst *M = dyn_cast<MemSetInst>(I))
>> - RepeatInstruction = processMemSet(M, BI);
>> + MadeChange = processMemSet(M, BI);
>> else if (MemCpyInst *M = dyn_cast<MemCpyInst>(I))
>> - RepeatInstruction = processMemCpy(M, BI);
>> + MadeChange = processMemCpy(M, BI);
>> else if (MemMoveInst *M = dyn_cast<MemMoveInst>(I))
>> - RepeatInstruction = processMemMove(M);
>> + MadeChange = processMemMove(M, BI);
>> else if (auto *CB = dyn_cast<CallBase>(I)) {
>> for (unsigned i = 0, e = CB->arg_size(); i != e; ++i)
>> if (CB->isByValArgument(i))
>> MadeChange |= processByValArgument(*CB, i);
>> }
>> -
>> - // Reprocess the instruction if desired.
>> - if (RepeatInstruction) {
>> - if (BI != BB.begin())
>> - --BI;
>> - MadeChange = true;
>> - }
>> }
>> }
>>
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200612/46c507fe/attachment.html>
More information about the llvm-commits
mailing list