[llvm] f79e6a8 - [MemCpyOptimizer] Simplify API of processStore and processMem* functions
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 12 13:34:10 PDT 2020
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/6221855f/attachment.html>
More information about the llvm-commits
mailing list