-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[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 12 commits
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,11 @@ static cl::opt<bool> | |||||||||
OptimizeMemorySSA("dse-optimize-memoryssa", cl::init(true), cl::Hidden, | ||||||||||
cl::desc("Allow DSE to optimize memory accesses.")); | ||||||||||
|
||||||||||
// TODO: turn on and remove this flag. | ||||||||||
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 +815,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 +828,60 @@ 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, | ||||||||||
ArrayRef<std::pair<MemoryLocation, bool>> MemLocations) { | ||||||||||
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) { | ||||||||||
CallBase *CB = dyn_cast<CallBase>(I); | ||||||||||
return CB != nullptr && | ||||||||||
CB->getArgOperandWithAttribute(Attribute::Initializes) != nullptr; | ||||||||||
|
return CB != nullptr && | |
CB->getArgOperandWithAttribute(Attribute::Initializes) != nullptr; | |
return CB && CB->getArgOperandWithAttribute(Attribute::Initializes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Outdated
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Returns a list of <MemoryLocation, bool> pairs wrote by I. | |
// Returns a list of <MemoryLocation, bool> pairs written by I. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Callee would be dominated by initializations, so this should be safe. | |
// the callee would be dominated by initializations, so this should be safe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// or the argument is invisble to caller on unwind. That is, we don't | |
// or the argument is invisible to caller on unwind. That is, we don't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is nounwind checked in getIntersectedInitRangeList, no need to check it in within IsDeadOrInvisibleOnUnwind as the others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsDeadOrInvisibleOnUnwind
is per argument nounwind.
getIntersectedInitRangeList
considers IsDeadOrInvisibleOnUnwind
per argument and CB->doesNotThrow() together.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe nit: should be implicit that it's not an Invoke if there are no unwind edges?
// - Or the argument is invisible to caller on unwind, and CB isa<CallInst> | |
// which means no unwind edges from this call in the current function. | |
// - Or the argument is invisible to the caller on unwind, and it's a pure function call, i.e., there are no | |
// unwind edges from this call in the current function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't heard the term "pure function call" refer to a call that doesn't unwind, I prefer something closer to the current version.
Or the argument is invisible to caller on unwind, and there are no unwind edges from this call in the current function (e.g. `CallInst`).
There are probably (?) more cases than just CallInst
where we do this optimization so I use e.g.
instead of i.e.
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you both, done!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(isInvisibleToCallerOnUnwind(CurArg) && isa<CallInst>(CB)); | |
(isa<CallInst>(CB) && isInvisibleToCallerOnUnwind(CurArg)); |
Cheaper check first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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!
Uh oh!
There was an error while loading. Please reload this page.