-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[DSE] Apply initializes attribute to DSE #107282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
a94a734
002d984
eed0dff
e8163c9
7e6f960
debf11f
72dcab3
f660110
e9c9941
634948e
11a9cd9
2277de0
1b8c278
c2db695
c855aec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -52,6 +52,7 @@ | |||||
#include "llvm/IR/Argument.h" | ||||||
#include "llvm/IR/BasicBlock.h" | ||||||
#include "llvm/IR/Constant.h" | ||||||
#include "llvm/IR/ConstantRangeList.h" | ||||||
#include "llvm/IR/Constants.h" | ||||||
#include "llvm/IR/DataLayout.h" | ||||||
#include "llvm/IR/DebugInfo.h" | ||||||
|
@@ -164,6 +165,10 @@ static cl::opt<bool> | |||||
OptimizeMemorySSA("dse-optimize-memoryssa", cl::init(true), cl::Hidden, | ||||||
cl::desc("Allow DSE to optimize memory accesses.")); | ||||||
|
||||||
static cl::opt<bool> EnableInitializesImprovement( | ||||||
"enable-dse-initializes-attr-improvement", cl::init(false), cl::Hidden, | ||||||
cl::desc("Enable the initializes attr improvement in DSE")); | ||||||
|
||||||
//===----------------------------------------------------------------------===// | ||||||
// Helper functions | ||||||
//===----------------------------------------------------------------------===// | ||||||
|
@@ -809,8 +814,10 @@ bool canSkipDef(MemoryDef *D, bool DefVisibleToCaller) { | |||||
// A memory location wrapper that represents a MemoryLocation, `MemLoc`, | ||||||
// defined by `MemDef`. | ||||||
struct MemoryLocationWrapper { | ||||||
MemoryLocationWrapper(MemoryLocation MemLoc, MemoryDef *MemDef) | ||||||
: MemLoc(MemLoc), MemDef(MemDef) { | ||||||
MemoryLocationWrapper(MemoryLocation MemLoc, MemoryDef *MemDef, | ||||||
bool DefByInitializesAttr) | ||||||
: MemLoc(MemLoc), MemDef(MemDef), | ||||||
DefByInitializesAttr(DefByInitializesAttr) { | ||||||
assert(MemLoc.Ptr && "MemLoc should be not null"); | ||||||
UnderlyingObject = getUnderlyingObject(MemLoc.Ptr); | ||||||
DefInst = MemDef->getMemoryInst(); | ||||||
|
@@ -820,20 +827,121 @@ struct MemoryLocationWrapper { | |||||
const Value *UnderlyingObject; | ||||||
MemoryDef *MemDef; | ||||||
Instruction *DefInst; | ||||||
bool DefByInitializesAttr = false; | ||||||
}; | ||||||
|
||||||
// A memory def wrapper that represents a MemoryDef and the MemoryLocation(s) | ||||||
// defined by this MemoryDef. | ||||||
struct MemoryDefWrapper { | ||||||
MemoryDefWrapper(MemoryDef *MemDef, std::optional<MemoryLocation> MemLoc) { | ||||||
MemoryDefWrapper( | ||||||
MemoryDef *MemDef, | ||||||
const SmallVectorImpl<std::pair<MemoryLocation, bool>> &MemLocations) { | ||||||
nikic marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
DefInst = MemDef->getMemoryInst(); | ||||||
if (MemLoc.has_value()) | ||||||
DefinedLocation = MemoryLocationWrapper(*MemLoc, MemDef); | ||||||
for (auto &[MemLoc, DefByInitializesAttr] : MemLocations) | ||||||
DefinedLocations.push_back( | ||||||
MemoryLocationWrapper(MemLoc, MemDef, DefByInitializesAttr)); | ||||||
} | ||||||
Instruction *DefInst; | ||||||
std::optional<MemoryLocationWrapper> DefinedLocation = std::nullopt; | ||||||
SmallVector<MemoryLocationWrapper, 1> DefinedLocations; | ||||||
}; | ||||||
|
||||||
bool HasInitializesAttr(Instruction *I) { | ||||||
jvoung marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
CallBase *CB = dyn_cast<CallBase>(I); | ||||||
if (!CB) | ||||||
return false; | ||||||
|
||||||
for (size_t Idx = 0; Idx < CB->arg_size(); Idx++) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here and ++Idx There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Thanks!
jvoung marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (CB->paramHasAttr(Idx, Attribute::Initializes)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct me if I'm wrong. Different from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you need to call it for both the call and function attribute list. Or add a new CallBase API that does this for you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
return true; | ||||||
return false; | ||||||
} | ||||||
|
||||||
struct ArgumentInitInfo { | ||||||
size_t Idx = -1; | ||||||
jvoung marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
ConstantRangeList Inits; | ||||||
bool HasDeadOnUnwindAttr = false; | ||||||
bool FuncHasNoUnwindAttr = false; | ||||||
jvoung marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
}; | ||||||
|
||||||
ConstantRangeList | ||||||
GetMergedInitAttr(const SmallVectorImpl<ArgumentInitInfo> &Args) { | ||||||
jvoung marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (Args.empty()) | ||||||
return {}; | ||||||
|
||||||
// To address unwind, the function should have nounwind attribute or the | ||||||
// arguments have dead_on_unwind attribute. Otherwise, return empty. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can you update the comment "the arguments have dead_on_unwind attribute"? I think now it is "dead or invisible on unwind"? Similar on line 862, and maybe the field "IsDeadOnUnwind" could be "IsDeadOrInvisibleOnUnwind"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||||||
for (const auto &Arg : Args) { | ||||||
if (!Arg.FuncHasNoUnwindAttr && !Arg.HasDeadOnUnwindAttr) | ||||||
return {}; | ||||||
if (Arg.Inits.empty()) | ||||||
return {}; | ||||||
} | ||||||
|
||||||
if (Args.size() == 1) | ||||||
return Args[0].Inits; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is redundant with the code below, I'd remove it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, done! |
||||||
|
||||||
ConstantRangeList MergedIntervals = Args[0].Inits; | ||||||
for (size_t i = 1; i < Args.size(); i++) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. stylish nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for this info! Will keep these style rules in mind. |
||||||
MergedIntervals = MergedIntervals.intersectWith(Args[i].Inits); | ||||||
|
||||||
return MergedIntervals; | ||||||
} | ||||||
|
||||||
// Return the locations wrote by the initializes attribute. | ||||||
nikic marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
// Note that this function considers: | ||||||
// 1. Unwind edge: apply "initializes" attribute only if the callee has | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||||||
// "nounwind" attribute or the argument has "dead_on_unwind" attribute. | ||||||
// 2. Argument alias: for aliasing arguments, the "initializes" attribute is | ||||||
// the merged range list of their "initializes" attributes. | ||||||
jvoung marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
SmallVector<MemoryLocation, 1> | ||||||
GetInitializesArgMemLoc(const Instruction *I, BatchAAResults &BatchAA) { | ||||||
const CallBase *CB = dyn_cast<CallBase>(I); | ||||||
if (!CB) | ||||||
return {}; | ||||||
|
||||||
// Collect aliasing arguments and their initializes ranges. | ||||||
bool HasNoUnwindAttr = CB->hasFnAttr(Attribute::NoUnwind); | ||||||
SmallMapVector<Value *, SmallVector<ArgumentInitInfo, 2>, 2> Arguments; | ||||||
for (size_t Idx = 0; Idx < CB->arg_size(); Idx++) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||||||
ConstantRangeList Inits; | ||||||
if (CB->paramHasAttr(Idx, Attribute::Initializes)) | ||||||
Inits = CB->getParamAttr(Idx, Attribute::Initializes) | ||||||
nikic marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
.getValueAsConstantRangeList(); | ||||||
|
||||||
bool HasDeadOnUnwindAttr = CB->paramHasAttr(Idx, Attribute::DeadOnUnwind); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be IIUC,
not
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nikic to double check my understanding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think both checks would be correct in this context, but checking isInvisibleToCallerOnUnwind is probably more useful, especially as dead_on_unwind is (at present) not an inferred attribute, so only checking it would e.g. not handle trivial cases like an alloca being passed to the argument. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we use
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point, hadn't really considered the unwind being handled in the function. So the current implementation checks the right thing. If necessary, we can infer dead_on_unwind from isInvisibleToCallerOnUnwind + unwind on the call is not handled, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @haopliu can you change this to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, good point. Thank you and done! Added a unit test, |
||||||
ArgumentInitInfo InitInfo{Idx, Inits, HasDeadOnUnwindAttr, HasNoUnwindAttr}; | ||||||
Value *CurArg = CB->getArgOperand(Idx); | ||||||
bool FoundAliasing = false; | ||||||
for (auto &[Arg, AliasList] : Arguments) { | ||||||
if (BatchAA.isMustAlias(Arg, CurArg)) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This handle MustAlias arguments, but what about arguments that MayAlias or PartialAlias? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Conservatively handle May-/Partial-Alias same as MustAlias. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just handling them the same as MustAlias isn't correct. If it's PartialAlias then there is an offset between the arguments, so initializes refers to different offsets. And if it's MayAlias, then there may be an unknown offset, or the arguments may be unrelated entirely. For the PartialAlias/MayAlias cases we should discard the initializes information entirely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, thanks for the reminder. Make sense! Updated to insert an empty range for May-/Partial-Alias. This empty range would discard the entire initializes info later while intersecting the ranges among all aliasing args. |
||||||
FoundAliasing = true; | ||||||
AliasList.push_back(InitInfo); | ||||||
} | ||||||
} | ||||||
if (!FoundAliasing) | ||||||
Arguments[CurArg] = {InitInfo}; | ||||||
} | ||||||
|
||||||
SmallVector<MemoryLocation, 1> Locations; | ||||||
for (const auto &[_, Args] : Arguments) { | ||||||
auto MergedInitAttr = GetMergedInitAttr(Args); | ||||||
if (MergedInitAttr.empty()) | ||||||
continue; | ||||||
|
||||||
for (const auto &Arg : Args) { | ||||||
for (const auto &Range : MergedInitAttr) { | ||||||
int64_t Start = Range.getLower().getSExtValue(); | ||||||
int64_t End = Range.getUpper().getSExtValue(); | ||||||
if (Start == 0) | ||||||
nikic marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
Locations.push_back(MemoryLocation(CB->getArgOperand(Arg.Idx), | ||||||
LocationSize::precise(End - Start), | ||||||
nikic marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
CB->getAAMetadata())); | ||||||
} | ||||||
} | ||||||
} | ||||||
return Locations; | ||||||
} | ||||||
|
||||||
struct DSEState { | ||||||
Function &F; | ||||||
AliasAnalysis &AA; | ||||||
|
@@ -911,7 +1019,8 @@ struct DSEState { | |||||
|
||||||
auto *MD = dyn_cast_or_null<MemoryDef>(MA); | ||||||
if (MD && MemDefs.size() < MemorySSADefsPerBlockLimit && | ||||||
(getLocForWrite(&I) || isMemTerminatorInst(&I))) | ||||||
(getLocForWrite(&I) || isMemTerminatorInst(&I) || | ||||||
HasInitializesAttr(&I))) | ||||||
jvoung marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
MemDefs.push_back(MD); | ||||||
} | ||||||
} | ||||||
|
@@ -1147,13 +1256,26 @@ struct DSEState { | |||||
return MemoryLocation::getOrNone(I); | ||||||
} | ||||||
|
||||||
std::optional<MemoryLocation> getLocForInst(Instruction *I) { | ||||||
// Returns a list of <MemoryLocation, bool> pairs wrote by I. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||||||
// The bool means whether the write is from Initializes attr. | ||||||
SmallVector<std::pair<MemoryLocation, bool>, 1> | ||||||
getLocForInst(Instruction *I, bool ConsiderInitializesAttr) { | ||||||
SmallVector<std::pair<MemoryLocation, bool>, 1> Locations; | ||||||
if (isMemTerminatorInst(I)) { | ||||||
if (auto Loc = getLocForTerminator(I)) { | ||||||
return Loc->first; | ||||||
if (auto Loc = getLocForTerminator(I)) | ||||||
Locations.push_back(std::make_pair(Loc->first, false)); | ||||||
return Locations; | ||||||
} | ||||||
|
||||||
if (auto Loc = getLocForWrite(I)) | ||||||
Locations.push_back(std::make_pair(*Loc, false)); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we return here or does this need to fall through? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Changed it to early return. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted the latest change. We need to fall through. For a call instruction, getLocForWrite may return a memory-location with imprecise size. Then, fall through to check the initializes attr. |
||||||
|
||||||
if (ConsiderInitializesAttr) { | ||||||
for (auto &MemLoc : GetInitializesArgMemLoc(I, BatchAA)) { | ||||||
Locations.push_back(std::make_pair(MemLoc, true)); | ||||||
} | ||||||
} | ||||||
return getLocForWrite(I); | ||||||
return Locations; | ||||||
} | ||||||
|
||||||
/// Assuming this instruction has a dead analyzable write, can we delete | ||||||
|
@@ -1365,7 +1487,8 @@ struct DSEState { | |||||
getDomMemoryDef(MemoryDef *KillingDef, MemoryAccess *StartAccess, | ||||||
const MemoryLocation &KillingLoc, const Value *KillingUndObj, | ||||||
unsigned &ScanLimit, unsigned &WalkerStepLimit, | ||||||
bool IsMemTerm, unsigned &PartialLimit) { | ||||||
bool IsMemTerm, unsigned &PartialLimit, | ||||||
bool IsInitializesAttrMemLoc) { | ||||||
if (ScanLimit == 0 || WalkerStepLimit == 0) { | ||||||
LLVM_DEBUG(dbgs() << "\n ... hit scan limit\n"); | ||||||
return std::nullopt; | ||||||
|
@@ -1602,7 +1725,17 @@ struct DSEState { | |||||
|
||||||
// Uses which may read the original MemoryDef mean we cannot eliminate the | ||||||
// original MD. Stop walk. | ||||||
if (isReadClobber(MaybeDeadLoc, UseInst)) { | ||||||
// If KillingDef is a CallInst with "initializes" attribute, the reads in | ||||||
// Callee would be dominated by initializations, so this should be safe. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||||||
bool IsKillingDefFromInitAttr = false; | ||||||
if (IsInitializesAttrMemLoc) { | ||||||
if (KillingI == UseInst && | ||||||
KillingUndObj == getUnderlyingObject(MaybeDeadLoc.Ptr)) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this inner if is untested (test pass if I replace the condition with true). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch! Added a new unit test, |
||||||
IsKillingDefFromInitAttr = true; | ||||||
} | ||||||
} | ||||||
|
||||||
if (isReadClobber(MaybeDeadLoc, UseInst) && !IsKillingDefFromInitAttr) { | ||||||
LLVM_DEBUG(dbgs() << " ... found read clobber\n"); | ||||||
return std::nullopt; | ||||||
} | ||||||
|
@@ -2207,7 +2340,8 @@ DSEState::eliminateDeadDefs(const MemoryLocationWrapper &KillingLocWrapper) { | |||||
std::optional<MemoryAccess *> MaybeDeadAccess = getDomMemoryDef( | ||||||
KillingLocWrapper.MemDef, Current, KillingLocWrapper.MemLoc, | ||||||
KillingLocWrapper.UnderlyingObject, ScanLimit, WalkerStepLimit, | ||||||
isMemTerminatorInst(KillingLocWrapper.DefInst), PartialLimit); | ||||||
isMemTerminatorInst(KillingLocWrapper.DefInst), PartialLimit, | ||||||
KillingLocWrapper.DefByInitializesAttr); | ||||||
|
||||||
if (!MaybeDeadAccess) { | ||||||
LLVM_DEBUG(dbgs() << " finished walk\n"); | ||||||
|
@@ -2232,8 +2366,11 @@ DSEState::eliminateDeadDefs(const MemoryLocationWrapper &KillingLocWrapper) { | |||||
} | ||||||
MemoryDefWrapper DeadDefWrapper( | ||||||
cast<MemoryDef>(DeadAccess), | ||||||
getLocForInst(cast<MemoryDef>(DeadAccess)->getMemoryInst())); | ||||||
MemoryLocationWrapper &DeadLocWrapper = *DeadDefWrapper.DefinedLocation; | ||||||
getLocForInst(cast<MemoryDef>(DeadAccess)->getMemoryInst(), | ||||||
/*ConsiderInitializesAttr=*/false)); | ||||||
assert(DeadDefWrapper.DefinedLocations.size() == 1); | ||||||
MemoryLocationWrapper &DeadLocWrapper = | ||||||
DeadDefWrapper.DefinedLocations.front(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not 100% sure what's going on here, but it seems weird that we have two modes for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We cannot apply the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. after staring at this a bit, I believe this preexisting code is conflating two things: it's assuming that if there is a memory location that I think ideally we change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this still needs a comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh, missed this comment. Added a TODO about isRemovable(). Thanks! |
||||||
LLVM_DEBUG(dbgs() << " (" << *DeadLocWrapper.DefInst << ")\n"); | ||||||
ToCheck.insert(DeadLocWrapper.MemDef->getDefiningAccess()); | ||||||
NumGetDomMemoryDefPassed++; | ||||||
|
@@ -2311,37 +2448,41 @@ DSEState::eliminateDeadDefs(const MemoryLocationWrapper &KillingLocWrapper) { | |||||
} | ||||||
|
||||||
bool DSEState::eliminateDeadDefs(const MemoryDefWrapper &KillingDefWrapper) { | ||||||
if (!KillingDefWrapper.DefinedLocation.has_value()) { | ||||||
if (KillingDefWrapper.DefinedLocations.empty()) { | ||||||
LLVM_DEBUG(dbgs() << "Failed to find analyzable write location for " | ||||||
<< *KillingDefWrapper.DefInst << "\n"); | ||||||
return false; | ||||||
} | ||||||
|
||||||
auto &KillingLocWrapper = *KillingDefWrapper.DefinedLocation; | ||||||
LLVM_DEBUG(dbgs() << "Trying to eliminate MemoryDefs killed by " | ||||||
<< *KillingLocWrapper.MemDef << " (" | ||||||
<< *KillingLocWrapper.DefInst << ")\n"); | ||||||
auto [Changed, DeletedKillingLoc] = eliminateDeadDefs(KillingLocWrapper); | ||||||
|
||||||
// Check if the store is a no-op. | ||||||
if (!DeletedKillingLoc && storeIsNoop(KillingLocWrapper.MemDef, | ||||||
KillingLocWrapper.UnderlyingObject)) { | ||||||
LLVM_DEBUG(dbgs() << "DSE: Remove No-Op Store:\n DEAD: " | ||||||
<< *KillingLocWrapper.DefInst << '\n'); | ||||||
deleteDeadInstruction(KillingLocWrapper.DefInst); | ||||||
NumRedundantStores++; | ||||||
return true; | ||||||
} | ||||||
// Can we form a calloc from a memset/malloc pair? | ||||||
if (!DeletedKillingLoc && | ||||||
tryFoldIntoCalloc(KillingLocWrapper.MemDef, | ||||||
KillingLocWrapper.UnderlyingObject)) { | ||||||
LLVM_DEBUG(dbgs() << "DSE: Remove memset after forming calloc:\n" | ||||||
<< " DEAD: " << *KillingLocWrapper.DefInst << '\n'); | ||||||
deleteDeadInstruction(KillingLocWrapper.DefInst); | ||||||
return true; | ||||||
bool MadeChange = false; | ||||||
for (auto &KillingLocWrapper : KillingDefWrapper.DefinedLocations) { | ||||||
LLVM_DEBUG(dbgs() << "Trying to eliminate MemoryDefs killed by " | ||||||
<< *KillingLocWrapper.MemDef << " (" | ||||||
<< *KillingLocWrapper.DefInst << ")\n"); | ||||||
auto [Changed, DeletedKillingLoc] = eliminateDeadDefs(KillingLocWrapper); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like we're not taking this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, nice catch! Is there a way to launch an offline buildbot run to validate? https://lab.llvm.org/buildbot/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you repro locally with the same cmake flags as the bot? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, I repro the failure locally and confirmed that |
||||||
|
||||||
// Check if the store is a no-op. | ||||||
if (!DeletedKillingLoc && storeIsNoop(KillingLocWrapper.MemDef, | ||||||
KillingLocWrapper.UnderlyingObject)) { | ||||||
LLVM_DEBUG(dbgs() << "DSE: Remove No-Op Store:\n DEAD: " | ||||||
<< *KillingLocWrapper.DefInst << '\n'); | ||||||
deleteDeadInstruction(KillingLocWrapper.DefInst); | ||||||
NumRedundantStores++; | ||||||
MadeChange = true; | ||||||
continue; | ||||||
} | ||||||
// Can we form a calloc from a memset/malloc pair? | ||||||
if (!DeletedKillingLoc && | ||||||
tryFoldIntoCalloc(KillingLocWrapper.MemDef, | ||||||
KillingLocWrapper.UnderlyingObject)) { | ||||||
LLVM_DEBUG(dbgs() << "DSE: Remove memset after forming calloc:\n" | ||||||
<< " DEAD: " << *KillingLocWrapper.DefInst << '\n'); | ||||||
deleteDeadInstruction(KillingLocWrapper.DefInst); | ||||||
MadeChange = true; | ||||||
continue; | ||||||
} | ||||||
} | ||||||
return Changed; | ||||||
return MadeChange; | ||||||
} | ||||||
|
||||||
static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA, | ||||||
|
@@ -2357,7 +2498,8 @@ static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA, | |||||
continue; | ||||||
|
||||||
MemoryDefWrapper KillingDefWrapper( | ||||||
KillingDef, State.getLocForInst(KillingDef->getMemoryInst())); | ||||||
KillingDef, State.getLocForInst(KillingDef->getMemoryInst(), | ||||||
EnableInitializesImprovement)); | ||||||
MadeChange |= State.eliminateDeadDefs(KillingDefWrapper); | ||||||
} | ||||||
|
||||||
|
Uh oh!
There was an error while loading. Please reload this page.