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