Skip to content
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.

Sign up for GitHub

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

[DSE] Apply initializes attribute to DSE #107282

Merged
merged 15 commits into from
Oct 24, 2024
Merged

[DSE] Apply initializes attribute to DSE #107282

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a94a734
Apply initializes attribute to DSE
haopliu Sep 4, 2024
002d984
Update comments and unit test
haopliu Sep 5, 2024
eed0dff
Fix loop format, index type, and clean up ArgumentInitInfo struct mem…
haopliu Sep 5, 2024
e8163c9
Change 'Count = Args.size()' to size_t type
haopliu Sep 5, 2024
7e6f960
Handle may/parital alias and clean up code
haopliu Sep 10, 2024
debf11f
Early return in getLocForInst()
haopliu Sep 10, 2024
72dcab3
Insert an empty init range for May/PartialAlias arguments
haopliu Sep 11, 2024
f660110
Fix a mistake in getLocForInst: add getLocForWith no matter ConsiderI…
haopliu Sep 12, 2024
e9c9941
Use MemoryLocation::getBeforeOrAfter() in BatchAA.isNoAlias(Value*, V…
haopliu Sep 12, 2024
634948e
Call BatchAA.alias() only once then check AAResult
haopliu Sep 14, 2024
11a9cd9
Consider isInvisibleToCallerOnUnwind while checking an argument in CB…
haopliu Oct 5, 2024
2277de0
Update comments and unit tests
haopliu Oct 12, 2024
1b8c278
Rename IsDeadOnUnwind to IsDeadOrInvisibleOnUnwind
haopliu Oct 18, 2024
c2db695
Add a TODO about isRemovable()
haopliu Oct 18, 2024
c855aec
Update comments
haopliu Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
226 changes: 184 additions & 42 deletions llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
aeubanks marked this conversation as resolved.
Show resolved Hide resolved
"enable-dse-initializes-attr-improvement", cl::init(false), cl::Hidden,
cl::desc("Enable the initializes attr improvement in DSE"));

//===----------------------------------------------------------------------===//
// Helper functions
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -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();
Expand All @@ -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++)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here and ++Idx

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an AttributeList::.hasAttrSomewhere API to do this check efficiently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong. Different from CallBase::paramHasAttr, AttributeList::.hasAttrSomewhere API does not look into the called function's parameter list so we cannot apply this API here.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CallBase::getArgOperandWithAttribute is exactly what we need (call AttributeList::.hasAttrSomewhere with both the instruction attr and the called function attr). Used this API :-D

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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is redundant with the code below, I'd remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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++)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stylish nit:
https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
and capital variable names:
for (size_t I = 0, Count = Args.size(); I < Count; ++I)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://llvm.org/docs/CodingStandards.html#prefer-preincrement
and
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: apply can be read as "adding" the attribute to the IR, perhaps use or take advantage of

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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++) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be isInvisibleToCallerOnUnwind()?

IIUC, dead_on_unwind means the caller of the call doesn't see the value on unwind. But the usage here is backwards, we're checking if the argument parameter to a call has dead_on_unwind. That means that this function won't read the result if the call unwinds. But what we really care about is that the caller of this function won't read the result on unwind. i.e. we care about

define @f(ptr dead_on_unwind %p) {
}

not

define @f(ptr %p) {
  call void @g(ptr dead_on_unwind %p)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic to double check my understanding

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we use isInvisibleToCallerOnUnwind, is it possible to have a case where we unwind from @g and see incorrect DSE in the unwind edge within @f?

define @f() {
  %a = alloca
  store to %a[0-4] ; incorrectly DSE'd?
  invoke @g(ptr initializes((0,4)) %a) to label %bb1 unwind label %bb2
bb2:
  load from %a
}

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@haopliu can you change this to be bool IsDeadOnUnwind = CB->paramHasAttr(Idx, Attribute::DeadOnUnwind) || (isInvisibleToCallerOnUnwind(CurArg) && isa<CallInst>(CB)); with an explanation about how we need to make sure that we don't perform incorrect DSE on unwind edges in the current function, and that isa<CallInst> means no unwind edges (maybe there's a better way to detect no unwind edges?)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good point. Thank you and done!

Added a unit test, p2_no_dead_on_unwind_but_invisble_to_caller_alias_caller, to test this change as well.

ArgumentInitInfo InitInfo{Idx, Inits, HasDeadOnUnwindAttr, HasNoUnwindAttr};
Value *CurArg = CB->getArgOperand(Idx);
bool FoundAliasing = false;
for (auto &[Arg, AliasList] : Arguments) {
if (BatchAA.isMustAlias(Arg, CurArg)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This handle MustAlias arguments, but what about arguments that MayAlias or PartialAlias?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conservatively handle May-/Partial-Alias same as MustAlias.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Returns a list of <MemoryLocation, bool> pairs wrote by I.
// Returns a list of <MemoryLocation, bool> pairs written by I.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we return here or does this need to fall through?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Changed it to early return.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Callee would be dominated by initializations, so this should be safe.
// the callee would be dominated by initializations, so this should be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Added a new unit test, p1_write_then_read_caller_with_clobber, to test this inner condition.

IsKillingDefFromInitAttr = true;
}
}

if (isReadClobber(MaybeDeadLoc, UseInst) && !IsKillingDefFromInitAttr) {
LLVM_DEBUG(dbgs() << " ... found read clobber\n");
return std::nullopt;
}
Expand Down Expand Up @@ -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");
Expand All @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The 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 MemoryDefWrapper, a single MemoryLocation version here and a multiple MemoryLocation below. is there any way to make this a little less hacked together? why does this have to be a single MemoryLocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot apply the initializes attribute to DeadAccess/DeadDef since it would consider a call instruction as dead store and remove it incorrectly. Added a comment to explain it. Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 DeadDefWrapper writes to that overlaps with KillingLocWrapper, it must have no other side effects and be deletable. this happens to be true for stores and libcalls like strcpy that are handled here, but is not necessarily true in general.

I think ideally we change isRemovable to be more accurate about arbitrary function calls, and check that here, but I'm ok with a TODO saying something like `TODO: this conflates the existence of a MemoryLocation with being able to delete the instruction. fix isRemovable() to consider calls with side effects that cannot be removed, e.g. calls with the initializes attribute, and remove getLocForInst(ConsiderInitializesAttr = false) workaround

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this still needs a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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++;
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we're not taking this Changed into account

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you repro locally with the same cmake flags as the bot?
E.g., -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON in this step https://lab.llvm.org/buildbot/#/builders/16/builds/7648/steps/4/logs/stdio ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I repro the failure locally and confirmed that MadeChange |= Changed; works.
Will retry this PR!


// 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,
Expand All @@ -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);
}

Expand Down
Loading
Loading