[DSE] Apply initializes attribute to DSE #107282
Conversation
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-transforms Author: Haopeng Liu (haopliu) ChangesApply the initializes attribute to DSE and guard with a flag, "enable-dse-initializes-attr-improvement". The attribute support has been landed in: #84803 Full diff: https://github.com/llvm/llvm-project/pull/107282.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index a37f295abbd31c..3ccb064adbf0df 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -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) {
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);
+ if (!CB)
+ return false;
+
+ for (size_t Idx = 0; Idx < CB->arg_size(); Idx++)
+ if (CB->paramHasAttr(Idx, Attribute::Initializes))
+ return true;
+ return false;
+}
+
+struct ArgumentInitInfo {
+ size_t Idx = -1;
+ ConstantRangeList Inits;
+ bool HasDeadOnUnwindAttr = false;
+ bool FuncHasNoUnwindAttr = false;
};
+ConstantRangeList
+GetMergedInitAttr(const SmallVectorImpl<ArgumentInitInfo> &Args) {
+ if (Args.empty())
+ return {};
+
+ // To address unwind, the function should have nounwind attribute or the
+ // arguments have dead_on_unwind attribute. Otherwise, return empty.
+ for (const auto &Arg : Args) {
+ if (!Arg.FuncHasNoUnwindAttr && !Arg.HasDeadOnUnwindAttr)
+ return {};
+ if (Arg.Inits.empty())
+ return {};
+ }
+
+ if (Args.size() == 1)
+ return Args[0].Inits;
+
+ ConstantRangeList MergedIntervals = Args[0].Inits;
+ for (size_t i = 1; i < Args.size(); i++)
+ MergedIntervals = MergedIntervals.intersectWith(Args[i].Inits);
+
+ return MergedIntervals;
+}
+
+// Return the locations wrote by the initializes attribute.
+// Note that this function considers:
+// 1. Unwind edge: apply "initializes" attribute only if the callee has
+// "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.
+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++) {
+ ConstantRangeList Inits;
+ if (CB->paramHasAttr(Idx, Attribute::Initializes))
+ Inits = CB->getParamAttr(Idx, Attribute::Initializes)
+ .getValueAsConstantRangeList();
+
+ bool HasDeadOnUnwindAttr = CB->paramHasAttr(Idx, Attribute::DeadOnUnwind);
+ ArgumentInitInfo InitInfo{Idx, Inits, HasDeadOnUnwindAttr, HasNoUnwindAttr};
+ Value *CurArg = CB->getArgOperand(Idx);
+ bool FoundAliasing = false;
+ for (auto &[Arg, AliasList] : Arguments) {
+ if (BatchAA.isMustAlias(Arg, CurArg)) {
+ 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)
+ Locations.push_back(MemoryLocation(CB->getArgOperand(Arg.Idx),
+ LocationSize::precise(End - Start),
+ 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)))
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.
+ // 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));
+
+ 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.
+ bool IsKillingDefFromInitAttr = false;
+ if (IsInitializesAttrMemLoc) {
+ if (KillingI == UseInst &&
+ KillingUndObj == getUnderlyingObject(MaybeDeadLoc.Ptr)) {
+ 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();
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);
+
+ // 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);
}
diff --git a/llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll b/llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll
new file mode 100644
index 00000000000000..c4ff69af9051bc
--- /dev/null
+++ b/llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll
@@ -0,0 +1,159 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -aa-pipeline=basic-aa -passes=function-attrs,dse -enable-dse-initializes-attr-improvement -S | FileCheck %s
+
+declare void @p1_write_only(ptr nocapture noundef writeonly initializes((0, 2)) dead_on_unwind)
+declare void @p1_write_then_read(ptr nocapture noundef initializes((0, 2)) dead_on_unwind)
+declare void @p2_same_range(ptr nocapture noundef initializes((0, 2)) dead_on_unwind, ptr nocapture noundef initializes((0, 2)) dead_on_unwind)
+declare void @p2_no_init(ptr nocapture noundef initializes((0, 2)) dead_on_unwind, ptr nocapture noundef dead_on_unwind)
+declare void @p2_no_dead_on_unwind(ptr nocapture noundef initializes((0, 2)) dead_on_unwind, ptr nocapture noundef initializes((0, 2)))
+
+; Function Attrs: mustprogress nounwind uwtable
+define i16 @p1_write_only_caller() {
+; CHECK-LABEL: @p1_write_only_caller(
+; CHECK-NEXT: %ptr = alloca i16, align 2
+; CHECK-NEXT: call void @p1_write_only(ptr %ptr)
+; CHECK-NEXT: %l = load i16, ptr %ptr
+; CHECK-NEXT: ret i16 %l
+;
+ %ptr = alloca i16
+ store i16 0, ptr %ptr
+ call void @p1_write_only(ptr %ptr)
+ %l = load i16, ptr %ptr
+ ret i16 %l
+}
+
+; Function Attrs: mustprogress nounwind uwtable
+define i16 @p1_write_then_read_caller() {
+; CHECK-LABEL: @p1_write_then_read_caller(
+; CHECK-NEXT: %ptr = alloca i16, align 2
+; CHECK-NEXT: call void @p1_write_then_read(ptr %ptr)
+; CHECK-NEXT: %l = load i16, ptr %ptr
+; CHECK-NEXT: ret i16 %l
+;
+ %ptr = alloca i16
+ store i16 0, ptr %ptr
+ call void @p1_write_then_read(ptr %ptr)
+ %l = load i16, ptr %ptr
+ ret i16 %l
+}
+
+; Function Attrs: mustprogress nounwind uwtable
+define i16 @p2_same_range_nonalias_caller() {
+; CHECK-LABEL: @p2_same_range_nonalias_caller(
+; CHECK-NEXT: %ptr1 = alloca i16, align 2
+; CHECK-NEXT: %ptr2 = alloca i16, align 2
+; CHECK-NEXT: call void @p2_same_range(ptr %ptr1, ptr %ptr2)
+; CHECK-NEXT: %l = load i16, ptr %ptr1
+; CHECK-NEXT: ret i16 %l
+;
+ %ptr1 = alloca i16
+ %ptr2 = alloca i16
+ store i16 0, ptr %ptr1
+ store i16 0, ptr %ptr2
+ call void @p2_same_range(ptr %ptr1, ptr %ptr2)
+ %l = load i16, ptr %ptr1
+ ret i16 %l
+}
+
+; Function Attrs: mustprogress nounwind uwtable
+define i16 @p2_same_range_alias_caller() {
+; CHECK-LABEL: @p2_same_range_alias_caller(
+; CHECK-NEXT: %ptr = alloca i16, align 2
+; CHECK-NEXT: call void @p2_same_range(ptr %ptr, ptr %ptr)
+; CHECK-NEXT: %l = load i16, ptr %ptr
+; CHECK-NEXT: ret i16 %l
+;
+ %ptr = alloca i16
+ store i16 0, ptr %ptr
+ call void @p2_same_range(ptr %ptr, ptr %ptr)
+ %l = load i16, ptr %ptr
+ ret i16 %l
+}
+
+; Function Attrs: mustprogress nounwind uwtable
+define i16 @p2_no_init_alias_caller() {
+; CHECK-LABEL: @p2_no_init_alias_caller(
+; CHECK-NEXT: %ptr = alloca i16, align 2
+; CHECK-NEXT: store i16 0, ptr %ptr
+; CHECK-NEXT: call void @p2_no_init(ptr %ptr, ptr %ptr)
+; CHECK-NEXT: %l = load i16, ptr %ptr
+; CHECK-NEXT: ret i16 %l
+;
+ %ptr = alloca i16
+ store i16 0, ptr %ptr
+ call void @p2_no_init(ptr %ptr, ptr %ptr)
+ %l = load i16, ptr %ptr
+ ret i16 %l
+}
+
+; Function Attrs: mustprogress nounwind uwtable
+define i16 @p2_no_dead_on_unwind_alias_caller() {
+; CHECK-LABEL: @p2_no_dead_on_unwind_alias_caller(
+; CHECK-NEXT: %ptr = alloca i16, align 2
+; CHECK-NEXT: store i16 0, ptr %ptr
+; CHECK-NEXT: call void @p2_no_dead_on_unwind(ptr %ptr, ptr %ptr)
+; CHECK-NEXT: %l = load i16, ptr %ptr
+; CHECK-NEXT: ret i16 %l
+;
+ %ptr = alloca i16
+ store i16 0, ptr %ptr
+ call void @p2_no_dead_on_unwind(ptr %ptr, ptr %ptr)
+ %l = load i16, ptr %ptr
+ ret i16 %l
+}
+
+declare void @llvm.memset.p0.i64(ptr nocapture, i8, i64, i1) nounwind
+declare void @large_p1(ptr nocapture noundef initializes((0, 200))) nounwind
+declare void @large_p2(ptr nocapture noundef initializes((0, 200)), ptr nocapture noundef initializes((0, 100))) nounwind
+
+; Function Attrs: mustprogress nounwind uwtable
+define i16 @large_p1_caller() {
+; CHECK-LABEL: @large_p1_caller(
+; CHECK-NEXT: %ptr = alloca i16, align 2
+; CHECK-NEXT: call void @large_p1(ptr %ptr)
+; CHECK-NEXT: %l = load i16, ptr %ptr
+; CHECK-NEXT: ret i16 %l
+;
+ %ptr = alloca i16
+ call void @llvm.memset.p0.i64(ptr %ptr, i8 42, i64 100, i1 false)
+ call void @large_p1(ptr %ptr)
+ %l = load i16, ptr %ptr
+ ret i16 %l
+}
+
+; Function Attrs: mustprogress nounwind uwtable
+define i16 @large_p2_nonalias_caller() {
+; CHECK-LABEL: @large_p2_nonalias_caller(
+; CHECK-NEXT: %ptr1 = alloca i16, align 2
+; CHECK-NEXT: %ptr2 = alloca i16, align 2
+; CHECK-NEXT: call void @large_p2(ptr %ptr1, ptr %ptr2)
+; CHECK-NEXT: %l = load i16, ptr %ptr1
+; CHECK-NEXT: ret i16 %l
+;
+ %ptr1 = alloca i16
+ %ptr2 = alloca i16
+ call void @llvm.memset.p0.i64(ptr %ptr1, i8 42, i64 200, i1 false)
+ call void @llvm.memset.p0.i64(ptr %ptr2, i8 42, i64 100, i1 false)
+ call void @large_p2(ptr %ptr1, ptr %ptr2)
+ %l = load i16, ptr %ptr1
+ ret i16 %l
+}
+
+
+; Function Attrs: mustprogress nounwind uwtable
+define i16 @large_p2_alias_caller() {
+; CHECK-LABEL: @large_p2_alias_caller(
+; CHECK-NEXT: %ptr = alloca i16, align 2
+; CHECK-NEXT: %1 = getelementptr inbounds i8, ptr %ptr, i64 100
+; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr align 1 %1, i8 42, i64 200, i1 false)
+; CHECK-NEXT: call void @large_p2(ptr %ptr, ptr %ptr)
+; CHECK-NEXT: %l = load i16, ptr %ptr
+; CHECK-NEXT: ret i16 %l
+;
+ %ptr = alloca i16
+ call void @llvm.memset.p0.i64(ptr %ptr, i8 42, i64 300, i1 false)
+ call void @large_p2(ptr %ptr, ptr %ptr)
+ %l = load i16, ptr %ptr
+ ret i16 %l
+}
+
|
| @@ -0,0 +1,159 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py | |||
| ; RUN: opt < %s -aa-pipeline=basic-aa -passes=function-attrs,dse -enable-dse-initializes-attr-improvement -S | FileCheck %s | |||
There was a problem hiding this comment.
is aa-pipeline necessary?
also this shouldn't be running function-attrs
There was a problem hiding this comment.
Oh, you are right. Removed the aa-pipeline and function-attrs.
| /*ConsiderInitializesAttr=*/false)); | ||
| assert(DeadDefWrapper.DefinedLocations.size() == 1); | ||
| MemoryLocationWrapper &DeadLocWrapper = | ||
| DeadDefWrapper.DefinedLocations.front(); |
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!
| return Args[0].Inits; | ||
|
|
||
| ConstantRangeList MergedIntervals = Args[0].Inits; | ||
| for (size_t i = 1; i < Args.size(); i++) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for this info! Will keep these style rules in mind.
| if (!CB) | ||
| return false; | ||
|
|
||
| for (size_t Idx = 0; Idx < CB->arg_size(); Idx++) |
| // 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++) { |
| return false; | ||
|
|
||
| for (unsigned Idx = 0, Count = CB->arg_size(); Idx < Count; ++Idx) | ||
| if (CB->paramHasAttr(Idx, Attribute::Initializes)) |
There was a problem hiding this comment.
There is an AttributeList::.hasAttrSomewhere API to do this check efficiently.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
CallBase::getArgOperandWithAttribute is exactly what we need (call AttributeList::.hasAttrSomewhere with both the instruction attr and the called function attr). Used this API :-D
| 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.
This handle MustAlias arguments, but what about arguments that MayAlias or PartialAlias?
There was a problem hiding this comment.
Conservatively handle May-/Partial-Alias same as MustAlias.
There was a problem hiding this comment.
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.
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.
| } | ||
|
|
||
| if (auto Loc = getLocForWrite(I)) | ||
| Locations.push_back(std::make_pair(*Loc, false)); |
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.
| bool IsKillingDefFromInitAttr = false; | ||
| if (IsInitializesAttrMemLoc) { | ||
| if (KillingI == UseInst && | ||
| KillingUndObj == getUnderlyingObject(MaybeDeadLoc.Ptr)) { |
There was a problem hiding this comment.
It looks like this inner if is untested (test pass if I replace the condition with true).
There was a problem hiding this comment.
Nice catch! Added a new unit test, p1_write_then_read_caller_with_clobber, to test this inner condition.
|
What does the compile-time impact for this look like? |
I think this PR (enable the flag) has a negligible compile-time impact. The diff between this PR (enable the flag) and adding the attribute inference is 0. We guard the change with a flag to confirm everything is expected after landing all, then enable this flag. |
…nitializesAttr is true or false
| for (auto &[Arg, AliasList] : Arguments) { | ||
| if (BatchAA.isNoAlias(Arg, CurArg)) { | ||
| continue; | ||
| } else if (BatchAA.isMustAlias(Arg, CurArg)) { |
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!
| if (InitializesAttr.isValid()) | ||
| Inits = InitializesAttr.getValueAsConstantRangeList(); | ||
|
|
||
| bool HasDeadOnUnwindAttr = CB->paramHasAttr(Idx, Attribute::DeadOnUnwind); |
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.
@nikic to double check my understanding
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.
| if (Args.size() == 1) | ||
| return Args[0].Inits; |
There was a problem hiding this comment.
this is redundant with the code below, I'd remove it
|
|
||
| // Return the locations written by the initializes attribute. | ||
| // Note that this function considers: | ||
| // 1. Unwind edge: apply "initializes" attribute only if the callee has |
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
… is dead on unwind
| /*ConsiderInitializesAttr=*/false)); | ||
| assert(DeadDefWrapper.DefinedLocations.size() == 1); | ||
| MemoryLocationWrapper &DeadLocWrapper = | ||
| DeadDefWrapper.DefinedLocations.front(); |
There was a problem hiding this comment.
this still needs a comment
|
|
||
| Value *CurArg = CB->getArgOperand(Idx); | ||
| // We don't perform incorrect DSE on unwind edges in the current function, | ||
| // and use the "initialize" attribute to kill dead stores if : |
There was a problem hiding this comment.
| // and use the "initialize" attribute to kill dead stores if : | |
| // and use the "initialize" attribute to kill dead stores if: |
| // and use the "initialize" attribute to kill dead stores if : | ||
| // - The call does not throw exceptions, "CB->doesNotThrow()". | ||
| // - Or the argument has "dead_on_unwind" attribute. | ||
| // - Or the argument is invisble to caller on unwind, and CB isa<CallInst> |
There was a problem hiding this comment.
| // - Or the argument is invisble to caller on unwind, and CB isa<CallInst> | |
| // - Or the argument is invisible to caller on unwind, and CB isa<CallInst> |
| // We don't perform incorrect DSE on unwind edges in the current function, | ||
| // and use the "initialize" attribute to kill dead stores if : | ||
| // - The call does not throw exceptions, "CB->doesNotThrow()". | ||
| // - Or the argument has "dead_on_unwind" attribute. |
There was a problem hiding this comment.
| // - Or the argument has "dead_on_unwind" attribute. | |
| // - Or the callee parameter has "dead_on_unwind" attribute. |
| // - The call does not throw exceptions, "CB->doesNotThrow()". | ||
| // - Or the argument has "dead_on_unwind" attribute. | ||
| // - Or the argument is invisble to caller on unwind, and CB isa<CallInst> | ||
| // which means no unwind edges. |
There was a problem hiding this comment.
| // which means no unwind edges. | |
| // which means no unwind edges from this call in the current function. |
| 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.
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"?
|
Thank you all!
Thanks for the reminder! Done. |
|
@nikic any more comments? |
| @@ -1147,13 +1196,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.
| // Returns a list of <MemoryLocation, bool> pairs wrote by I. | |
| // Returns a list of <MemoryLocation, bool> pairs written by I. |
| // which means no unwind edges from this call in the current function. | ||
| bool IsDeadOrInvisibleOnUnwind = | ||
| CB->paramHasAttr(Idx, Attribute::DeadOnUnwind) || | ||
| (isInvisibleToCallerOnUnwind(CurArg) && isa<CallInst>(CB)); |
There was a problem hiding this comment.
| (isInvisibleToCallerOnUnwind(CurArg) && isa<CallInst>(CB)); | |
| (isa<CallInst>(CB) && isInvisibleToCallerOnUnwind(CurArg)); |
Cheaper check first.
| return CB != nullptr && | ||
| CB->getArgOperandWithAttribute(Attribute::Initializes) != nullptr; |
There was a problem hiding this comment.
Maybe more readable?
| return CB != nullptr && | |
| CB->getArgOperandWithAttribute(Attribute::Initializes) != nullptr; | |
| return CB && CB->getArgOperandWithAttribute(Attribute::Initializes); |
| @@ -1602,7 +1665,16 @@ 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.
| // Callee would be dominated by initializations, so this should be safe. | |
| // the callee would be dominated by initializations, so this should be safe. |
| // Note that this function considers: | ||
| // 1. Unwind edge: use "initializes" attribute only if the callee has | ||
| // "nounwind" attribute, or the argument has "dead_on_unwind" attribute, | ||
| // or the argument is invisble to caller on unwind. That is, we don't |
There was a problem hiding this comment.
| // 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 |
| Value *CurArg = CB->getArgOperand(Idx); | ||
| // We don't perform incorrect DSE on unwind edges in the current function, | ||
| // and use the "initializes" attribute to kill dead stores if: | ||
| // - The call does not throw exceptions, "CB->doesNotThrow()". |
There was a problem hiding this comment.
Is nounwind checked in getIntersectedInitRangeList, no need to check it in within IsDeadOrInvisibleOnUnwind as the others?
There was a problem hiding this comment.
IsDeadOrInvisibleOnUnwind is per argument nounwind.
getIntersectedInitRangeList considers IsDeadOrInvisibleOnUnwind per argument and CB->doesNotThrow() together.
| // - 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. |
There was a problem hiding this comment.
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.
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.
Thank you both, done!
|
Thank you all! I'm going to merge this PR. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/7648 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/187/builds/2087 Here is the relevant piece of the build log for the reference |
|
@haopliu : As the build bots say, many lit tests fail when I've compiled with EXPENSIVE_CHECKS on. |
This reverts commit 089237c.
|
The revert went to a branch instead of |
Reverts #107282 Seems to be causing invalid analysis caching as mentioned in #107282 (comment).
|
reverted in #113589 |
| 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.
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!
retry #107282 Fixed with `MadeChange |= Changed;` and confirmed it works. ``` cmake -DLLVM_CCACHE_BUILD=ON -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON -DLLVM_ENABLE_WERROR=OFF -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS=-U_GLIBCXX_DEBUG '-DLLVM_LIT_ARGS=-v -vv -j96' '-DLLVM_ENABLE_PROJECTS=llvm;lld' -DLLVM_ENABLE_ASSERTIONS=ON -GNinja ../llvm ninja check-llvm ```
|
Re-merged this PR in #113630 with a fix. |
Apply the initializes attribute to DSE and guard with a flag, "enable-dse-initializes-attr-improvement".
The attribute support has been landed in: #84803
The attribute inference will be landed after this PR: #97373