[DSE] Apply initializes attribute to DSE #107282
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, | ||||||||
| 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,128 @@ 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, | ||||||||
| ArrayRef<std::pair<MemoryLocation, bool>> MemLocations) { | ||||||||
| DefInst = MemDef->getMemoryInst(); | ||||||||
| for (auto &[MemLoc, DefByInitializesAttr] : MemLocations) | ||||||||
| DefinedLocations.push_back( | ||||||||
| MemoryLocationWrapper(MemLoc, MemDef, DefByInitializesAttr)); | ||||||||
| } | ||||||||
| Instruction *DefInst; | ||||||||
| 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); |
Outdated
There was a problem hiding this comment.
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"?
Outdated
There was a problem hiding this comment.
this is redundant with the code below, I'd remove it
Outdated
There was a problem hiding this comment.
nit: apply can be read as "adding" the attribute to the IR, perhaps use or take advantage of
Outdated
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
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.
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
}
There was a problem hiding this comment.
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.
@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?)?
There was a problem hiding this comment.
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.
Outdated
There was a problem hiding this comment.
You should call alias() only once and then check the result.
There was a problem hiding this comment.
(Correct me if I'm wrong). Here checks two Value* aliasing: we need to convert them to two MemoryLocation, then call alias(). However, it seems that isMustAlias() and isNoAlias() convert in different ways: MemoryLocation(V, LocationSize::precise(1)) VS. MemoryLocation::getBeforeOrAfter(V). Then we have to call alias() twice?
There was a problem hiding this comment.
For MustAlias the size does not matter. Using getBeforeOrAfter() is always a safe option.
There was a problem hiding this comment.
I see. Updated to call BatchAA.alias() using getBeforeOrAfter() once, then check AAR. Thanks!
nikic marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
nikic marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
nikic marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
| // 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.
Can we return here or does this need to fall through?
There was a problem hiding this comment.
Done. Changed it to early return.
There was a problem hiding this comment.
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.
| // 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.
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.
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.
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.
this still needs a comment
There was a problem hiding this comment.
Ooh, missed this comment. Added a TODO about isRemovable(). Thanks!
There was a problem hiding this comment.
looks like we're not taking this Changed into account
There was a problem hiding this comment.
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.
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.
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.