Revert "[CaptureTracking] Ignore ephemeral values when determining po… #71066
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…inter escapeness" Unfortunately the commit (D123162) introduced a mis-compile (llvm#70547), which wasn't fixed by the alternative fix (c0de28b) I think as long as the call considered as ephemeral is not removed, we need to be conservative. To address the correctness issue quickly, I think we should revert the patch (as this patch does, it doens't revert cleanly) This reverts commit 17fdacc. Fixes llvm#70547
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: Florian Hahn (fhahn) ChangesUnfortunately the commit (D123162) introduced a mis-compile (#70547), which wasn't fixed by the alternative fix (c0de28b) I think as long as the call considered as ephemeral is not removed, we need to be conservative. To address the correctness issue quickly, I think we should revert the patch (as this patch does, it doens't revert cleanly) This reverts commit 17fdacc. Fixes #70547 Full diff: https://github.com/llvm/llvm-project/pull/71066.diff 6 Files Affected:
diff --git a/llvm/include/llvm/Analysis/AliasAnalysis.h b/llvm/include/llvm/Analysis/AliasAnalysis.h
index 9bcf9713787509b..0d0e94a89976a79 100644
--- a/llvm/include/llvm/Analysis/AliasAnalysis.h
+++ b/llvm/include/llvm/Analysis/AliasAnalysis.h
@@ -184,12 +184,9 @@ class EarliestEscapeInfo final : public CaptureInfo {
/// This is used for cache invalidation purposes.
DenseMap<Instruction *, TinyPtrVector<const Value *>> Inst2Obj;
- const SmallPtrSetImpl<const Value *> *EphValues;
-
public:
- EarliestEscapeInfo(DominatorTree &DT, const LoopInfo *LI = nullptr,
- const SmallPtrSetImpl<const Value *> *EphValues = nullptr)
- : DT(DT), LI(LI), EphValues(EphValues) {}
+ EarliestEscapeInfo(DominatorTree &DT, const LoopInfo *LI = nullptr)
+ : DT(DT), LI(LI) {}
bool isNotCapturedBeforeOrAt(const Value *Object,
const Instruction *I) override;
diff --git a/llvm/include/llvm/Analysis/CaptureTracking.h b/llvm/include/llvm/Analysis/CaptureTracking.h
index 6589c2b86363c0a..1fc7f34ceb55bac 100644
--- a/llvm/include/llvm/Analysis/CaptureTracking.h
+++ b/llvm/include/llvm/Analysis/CaptureTracking.h
@@ -25,7 +25,6 @@ namespace llvm {
class DominatorTree;
class LoopInfo;
class Function;
- template <typename T> class SmallPtrSetImpl;
/// getDefaultMaxUsesToExploreForCaptureTracking - Return default value of
/// the maximal number of uses to explore before giving up. It is used by
@@ -42,14 +41,8 @@ namespace llvm {
/// MaxUsesToExplore specifies how many uses the analysis should explore for
/// one value before giving up due too "too many uses". If MaxUsesToExplore
/// is zero, a default value is assumed.
- bool PointerMayBeCaptured(const Value *V, bool ReturnCaptures,
- bool StoreCaptures, unsigned MaxUsesToExplore = 0);
-
- /// Variant of the above function which accepts a set of Values that are
- /// ephemeral and cannot cause pointers to escape.
bool PointerMayBeCaptured(const Value *V, bool ReturnCaptures,
bool StoreCaptures,
- const SmallPtrSetImpl<const Value *> &EphValues,
unsigned MaxUsesToExplore = 0);
/// PointerMayBeCapturedBefore - Return true if this pointer value may be
@@ -83,7 +76,6 @@ namespace llvm {
Instruction *
FindEarliestCapture(const Value *V, Function &F, bool ReturnCaptures,
bool StoreCaptures, const DominatorTree &DT,
- const SmallPtrSetImpl<const Value *> *EphValues = nullptr,
unsigned MaxUsesToExplore = 0);
/// This callback is used in conjunction with PointerMayBeCaptured. In
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index f70b39b4f51a77a..a369ebe82320a15 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -208,7 +208,7 @@ bool EarliestEscapeInfo::isNotCapturedBeforeOrAt(const Value *Object,
if (Iter.second) {
Instruction *EarliestCapture = FindEarliestCapture(
Object, *const_cast<Function *>(I->getFunction()),
- /*ReturnCaptures=*/false, /*StoreCaptures=*/true, DT, EphValues);
+ /*ReturnCaptures=*/false, /*StoreCaptures=*/true, DT);
if (EarliestCapture) {
auto Ins = Inst2Obj.insert({EarliestCapture, {}});
Ins.first->second.push_back(Object);
diff --git a/llvm/lib/Analysis/CaptureTracking.cpp b/llvm/lib/Analysis/CaptureTracking.cpp
index 2b93620548341a6..eb120cb8241a231 100644
--- a/llvm/lib/Analysis/CaptureTracking.cpp
+++ b/llvm/lib/Analysis/CaptureTracking.cpp
@@ -16,7 +16,6 @@
//===----------------------------------------------------------------------===//
#include "llvm/Analysis/CaptureTracking.h"
-#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/Statistic.h"
@@ -74,10 +73,8 @@ bool CaptureTracker::isDereferenceableOrNull(Value *O, const DataLayout &DL) {
namespace {
struct SimpleCaptureTracker : public CaptureTracker {
- explicit SimpleCaptureTracker(
-
- const SmallPtrSetImpl<const Value *> &EphValues, bool ReturnCaptures)
- : EphValues(EphValues), ReturnCaptures(ReturnCaptures) {}
+ explicit SimpleCaptureTracker(bool ReturnCaptures)
+ : ReturnCaptures(ReturnCaptures) {}
void tooManyUses() override {
LLVM_DEBUG(dbgs() << "Captured due to too many uses\n");
@@ -88,17 +85,12 @@ namespace {
if (isa<ReturnInst>(U->getUser()) && !ReturnCaptures)
return false;
- if (EphValues.contains(U->getUser()))
- return false;
-
LLVM_DEBUG(dbgs() << "Captured by: " << *U->getUser() << "\n");
Captured = true;
return true;
}
- const SmallPtrSetImpl<const Value *> &EphValues;
-
bool ReturnCaptures;
bool Captured = false;
@@ -166,9 +158,8 @@ namespace {
// escape are not in a cycle.
struct EarliestCaptures : public CaptureTracker {
- EarliestCaptures(bool ReturnCaptures, Function &F, const DominatorTree &DT,
- const SmallPtrSetImpl<const Value *> *EphValues)
- : EphValues(EphValues), DT(DT), ReturnCaptures(ReturnCaptures), F(F) {}
+ EarliestCaptures(bool ReturnCaptures, Function &F, const DominatorTree &DT)
+ : DT(DT), ReturnCaptures(ReturnCaptures), F(F) {}
void tooManyUses() override {
Captured = true;
@@ -180,9 +171,6 @@ namespace {
if (isa<ReturnInst>(I) && !ReturnCaptures)
return false;
- if (EphValues && EphValues->contains(I))
- return false;
-
if (!EarliestCapture)
EarliestCapture = I;
else
@@ -194,8 +182,6 @@ namespace {
return false;
}
- const SmallPtrSetImpl<const Value *> *EphValues;
-
Instruction *EarliestCapture = nullptr;
const DominatorTree &DT;
@@ -217,17 +203,6 @@ namespace {
/// counts as capturing it or not.
bool llvm::PointerMayBeCaptured(const Value *V, bool ReturnCaptures,
bool StoreCaptures, unsigned MaxUsesToExplore) {
- SmallPtrSet<const Value *, 1> Empty;
- return PointerMayBeCaptured(V, ReturnCaptures, StoreCaptures, Empty,
- MaxUsesToExplore);
-}
-
-/// Variant of the above function which accepts a set of Values that are
-/// ephemeral and cannot cause pointers to escape.
-bool llvm::PointerMayBeCaptured(const Value *V, bool ReturnCaptures,
- bool StoreCaptures,
- const SmallPtrSetImpl<const Value *> &EphValues,
- unsigned MaxUsesToExplore) {
assert(!isa<GlobalValue>(V) &&
"It doesn't make sense to ask whether a global is captured.");
@@ -239,7 +214,7 @@ bool llvm::PointerMayBeCaptured(const Value *V, bool ReturnCaptures,
LLVM_DEBUG(dbgs() << "Captured?: " << *V << " = ");
- SimpleCaptureTracker SCT(EphValues, ReturnCaptures);
+ SimpleCaptureTracker SCT(ReturnCaptures);
PointerMayBeCaptured(V, &SCT, MaxUsesToExplore);
if (SCT.Captured)
++NumCaptured;
@@ -286,12 +261,11 @@ bool llvm::PointerMayBeCapturedBefore(const Value *V, bool ReturnCaptures,
Instruction *
llvm::FindEarliestCapture(const Value *V, Function &F, bool ReturnCaptures,
bool StoreCaptures, const DominatorTree &DT,
- const SmallPtrSetImpl<const Value *> *EphValues,
unsigned MaxUsesToExplore) {
assert(!isa<GlobalValue>(V) &&
"It doesn't make sense to ask whether a global is captured.");
- EarliestCaptures CB(ReturnCaptures, F, DT, EphValues);
+ EarliestCaptures CB(ReturnCaptures, F, DT);
PointerMayBeCaptured(V, &CB, MaxUsesToExplore);
if (CB.Captured)
++NumCapturedBefore;
diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index cacefc9718f1176..a7b2c95e5b2c8bc 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -38,9 +38,7 @@
#include "llvm/ADT/Statistic.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Analysis/AliasAnalysis.h"
-#include "llvm/Analysis/AssumptionCache.h"
#include "llvm/Analysis/CaptureTracking.h"
-#include "llvm/Analysis/CodeMetrics.h"
#include "llvm/Analysis/GlobalsModRef.h"
#include "llvm/Analysis/LoopInfo.h"
#include "llvm/Analysis/MemoryBuiltins.h"
@@ -842,9 +840,6 @@ struct DSEState {
// Post-order numbers for each basic block. Used to figure out if memory
// accesses are executed before another access.
DenseMap<BasicBlock *, unsigned> PostOrderNumbers;
- // Values that are only used with assumes. Used to refine pointer escape
- // analysis.
- SmallPtrSet<const Value *, 32> EphValues;
/// Keep track of instructions (partly) overlapping with killing MemoryDefs per
/// basic block.
@@ -864,10 +859,10 @@ struct DSEState {
DSEState &operator=(const DSEState &) = delete;
DSEState(Function &F, AliasAnalysis &AA, MemorySSA &MSSA, DominatorTree &DT,
- PostDominatorTree &PDT, AssumptionCache &AC,
- const TargetLibraryInfo &TLI, const LoopInfo &LI)
- : F(F), AA(AA), EI(DT, &LI, &EphValues), BatchAA(AA, &EI), MSSA(MSSA),
- DT(DT), PDT(PDT), TLI(TLI), DL(F.getParent()->getDataLayout()), LI(LI) {
+ PostDominatorTree &PDT, const TargetLibraryInfo &TLI,
+ const LoopInfo &LI)
+ : F(F), AA(AA), EI(DT, &LI), BatchAA(AA, &EI), MSSA(MSSA), DT(DT),
+ PDT(PDT), TLI(TLI), DL(F.getParent()->getDataLayout()), LI(LI) {
// Collect blocks with throwing instructions not modeled in MemorySSA and
// alloc-like objects.
unsigned PO = 0;
@@ -897,8 +892,6 @@ struct DSEState {
AnyUnreachableExit = any_of(PDT.roots(), [](const BasicBlock *E) {
return isa<UnreachableInst>(E->getTerminator());
});
-
- CodeMetrics::collectEphemeralValues(&F, &AC, EphValues);
}
LocationSize strengthenLocationSize(const Instruction *I,
@@ -1075,7 +1068,7 @@ struct DSEState {
if (!isInvisibleToCallerOnUnwind(V)) {
I.first->second = false;
} else if (isNoAliasCall(V)) {
- I.first->second = !PointerMayBeCaptured(V, true, false, EphValues);
+ I.first->second = !PointerMayBeCaptured(V, true, false);
}
}
return I.first->second;
@@ -1094,7 +1087,7 @@ struct DSEState {
// with the killing MemoryDef. But we refrain from doing so for now to
// limit compile-time and this does not cause any changes to the number
// of stores removed on a large test set in practice.
- I.first->second = PointerMayBeCaptured(V, false, true, EphValues);
+ I.first->second = PointerMayBeCaptured(V, false, true);
return !I.first->second;
}
@@ -2065,12 +2058,11 @@ struct DSEState {
static bool eliminateDeadStores(Function &F, AliasAnalysis &AA, MemorySSA &MSSA,
DominatorTree &DT, PostDominatorTree &PDT,
- AssumptionCache &AC,
const TargetLibraryInfo &TLI,
const LoopInfo &LI) {
bool MadeChange = false;
- DSEState State(F, AA, MSSA, DT, PDT, AC, TLI, LI);
+ DSEState State(F, AA, MSSA, DT, PDT, TLI, LI);
// For each store:
for (unsigned I = 0; I < State.MemDefs.size(); I++) {
MemoryDef *KillingDef = State.MemDefs[I];
@@ -2251,10 +2243,9 @@ PreservedAnalyses DSEPass::run(Function &F, FunctionAnalysisManager &AM) {
DominatorTree &DT = AM.getResult<DominatorTreeAnalysis>(F);
MemorySSA &MSSA = AM.getResult<MemorySSAAnalysis>(F).getMSSA();
PostDominatorTree &PDT = AM.getResult<PostDominatorTreeAnalysis>(F);
- AssumptionCache &AC = AM.getResult<AssumptionAnalysis>(F);
LoopInfo &LI = AM.getResult<LoopAnalysis>(F);
- bool Changed = eliminateDeadStores(F, AA, MSSA, DT, PDT, AC, TLI, LI);
+ bool Changed = eliminateDeadStores(F, AA, MSSA, DT, PDT, TLI, LI);
#ifdef LLVM_ENABLE_STATS
if (AreStatisticsEnabled())
diff --git a/llvm/test/Transforms/DeadStoreElimination/assume.ll b/llvm/test/Transforms/DeadStoreElimination/assume.ll
index c1c4027ea2f9a28..ddf61542b903b48 100644
--- a/llvm/test/Transforms/DeadStoreElimination/assume.ll
+++ b/llvm/test/Transforms/DeadStoreElimination/assume.ll
@@ -8,6 +8,7 @@ define void @f() {
; CHECK-NEXT: [[TMP1:%.*]] = call noalias ptr @_Znwm(i64 32)
; CHECK-NEXT: [[TMP2:%.*]] = icmp ugt ptr [[TMP1]], @global
; CHECK-NEXT: call void @llvm.assume(i1 [[TMP2]])
+; CHECK-NEXT: store i8 0, ptr [[TMP1]], align 1
; CHECK-NEXT: ret void
;
%tmp1 = call noalias ptr @_Znwm(i64 32)
@@ -22,6 +23,7 @@ define void @f2() {
; CHECK-NEXT: [[TMP1:%.*]] = call noalias ptr @_Znwm(i64 32)
; CHECK-NEXT: [[TMP2:%.*]] = icmp ugt ptr [[TMP1]], @global
; CHECK-NEXT: call void @llvm.assume(i1 [[TMP2]])
+; CHECK-NEXT: store i8 0, ptr [[TMP1]], align 1
; CHECK-NEXT: call void @quux(ptr @global)
; CHECK-NEXT: ret void
;
@@ -37,6 +39,7 @@ define void @f2() {
define void @pr70547() {
; CHECK-LABEL: @pr70547(
; CHECK-NEXT: [[A:%.*]] = alloca i8, align 1
+; CHECK-NEXT: store i8 0, ptr [[A]], align 1
; CHECK-NEXT: [[CALL:%.*]] = call ptr @quux(ptr [[A]]) #[[ATTR1:[0-9]+]]
; CHECK-NEXT: [[V:%.*]] = load i8, ptr [[CALL]], align 1
; CHECK-NEXT: [[CMP:%.*]] = icmp ne i8 [[V]], 1
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
aeubanks
approved these changes
Nov 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unfortunately the commit (D123162) introduced a mis-compile (#70547), which wasn't fixed by the alternative fix (c0de28b)
I think as long as the call considered as ephemeral is not removed, we need to be conservative. To address the correctness issue quickly, I think we should revert the patch (as this patch does, it doens't revert cleanly)
This reverts commit 17fdacc.
Fixes #70547