[llvm] Revert "[CaptureTracking] Ignore ephemeral values when determining po… (PR #71066)
Florian Hahn via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 2 07:58:59 PDT 2023
https://github.com/fhahn created https://github.com/llvm/llvm-project/pull/71066
Unfortunately the commit (D123162) introduced a mis-compile (https://github.com/llvm/llvm-project/issues/70547), which wasn't fixed by the alternative fix (c0de28b92e98acbeb73)
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 17fdaccccfad9b143e4aadbcdda7f645de127153.
Fixes https://github.com/llvm/llvm-project/issues/70547
>From f3ad47d25bc7a84d2677e4a71833439ec9684f9f Mon Sep 17 00:00:00 2001
From: Florian Hahn <flo at fhahn.com>
Date: Thu, 2 Nov 2023 14:51:26 +0000
Subject: [PATCH] Revert "[CaptureTracking] Ignore ephemeral values when
determining pointer escapeness"
Unfortunately the commit (D123162) introduced a mis-compile
(https://github.com/llvm/llvm-project/issues/70547), which wasn't fixed
by the alternative fix (c0de28b92e98acbeb73)
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 17fdaccccfad9b143e4aadbcdda7f645de127153.
Fixes https://github.com/llvm/llvm-project/issues/70547
---
llvm/include/llvm/Analysis/AliasAnalysis.h | 7 +---
llvm/include/llvm/Analysis/CaptureTracking.h | 8 ----
llvm/lib/Analysis/BasicAliasAnalysis.cpp | 2 +-
llvm/lib/Analysis/CaptureTracking.cpp | 38 +++----------------
.../Scalar/DeadStoreElimination.cpp | 25 ++++--------
.../Transforms/DeadStoreElimination/assume.ll | 3 ++
6 files changed, 20 insertions(+), 63 deletions(-)
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
More information about the llvm-commits
mailing list