[llvm] 6c269a3 - [BasicAA] Replace VisitedPhiBBs with a single flag
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 27 01:29:49 PDT 2022
Author: Nikita Popov
Date: 2022-10-27T10:29:41+02:00
New Revision: 6c269a3f89edde33a472d53ed017c1adfb62afcb
URL: https://github.com/llvm/llvm-project/commit/6c269a3f89edde33a472d53ed017c1adfb62afcb
DIFF: https://github.com/llvm/llvm-project/commit/6c269a3f89edde33a472d53ed017c1adfb62afcb.diff
LOG: [BasicAA] Replace VisitedPhiBBs with a single flag
When looking through phis, BasicAA has to guard against the
possibility that values from two separate cycle iterations are
being compared -- in this case, even though the SSA values may
be the same, they cannot be considered as equal.
This is currently done by keeping a set of VisitedPhiBBs for any
phis we looked through, and then checking whether the relevant
instruction is reachable from one of the phis.
This patch replaces this set with a single flag. If the flag is
set, then we will not assume equality for any instruction part
of a cycle. While this is nominally less accurate, it makes
essentially no difference in practice. Here are the AA stats
for test-suite:
aa.NumMayAlias | 3072005 | 3072016
aa.NumMustAlias | 337858 | 337854
aa.NumNoAlias | 13255345 | 13255349
The motivation for the change is to expose the MayBeCrossIteration
flag to AA users, which will allow fixing miscompiles related to
incorrect handling of cross-iteration AA queries.
Differential Revision: https://reviews.llvm.org/D136174
Added:
Modified:
llvm/include/llvm/Analysis/BasicAliasAnalysis.h
llvm/lib/Analysis/BasicAliasAnalysis.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Analysis/BasicAliasAnalysis.h b/llvm/include/llvm/Analysis/BasicAliasAnalysis.h
index d775366ab899a..8f4a8d4a8d8b2 100644
--- a/llvm/include/llvm/Analysis/BasicAliasAnalysis.h
+++ b/llvm/include/llvm/Analysis/BasicAliasAnalysis.h
@@ -92,7 +92,7 @@ class BasicAAResult : public AAResultBase {
private:
struct DecomposedGEP;
- /// Tracks phi nodes we have visited.
+ /// Tracks whether the accesses may be on
diff erent cycle iterations.
///
/// When interpret "Value" pointer equality as value equality we need to make
/// sure that the "Value" is not part of a cycle. Otherwise, two uses could
@@ -106,7 +106,7 @@ class BasicAAResult : public AAResultBase {
/// %addr2 = gep %alloca2, 0, (%l + 1)
/// alias(%p, %addr1) -> MayAlias !
/// store %l, ...
- SmallPtrSet<const BasicBlock *, 8> VisitedPhiBBs;
+ bool MayBeCrossIteration = false;
/// Tracks instructions visited by pointsToConstantMemory.
SmallPtrSet<const Value *, 16> Visited;
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index cca539499c06b..19ff4ba66a959 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -54,6 +54,7 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/KnownBits.h"
+#include "llvm/Support/SaveAndRestore.h"
#include <cassert>
#include <cstdint>
#include <cstdlib>
@@ -1252,12 +1253,12 @@ AliasResult BasicAAResult::aliasGEP(
} else if (DecompGEP1.VarIndices.size() == 2) {
// VarIndex = Scale*V0 + (-Scale)*V1.
// If V0 != V1 then abs(VarIndex) >= abs(Scale).
- // Check that VisitedPhiBBs is empty, to avoid reasoning about
+ // Check that MayBeCrossIteration is false, to avoid reasoning about
// inequality of values across loop iterations.
const VariableGEPIndex &Var0 = DecompGEP1.VarIndices[0];
const VariableGEPIndex &Var1 = DecompGEP1.VarIndices[1];
if (Var0.Scale == -Var1.Scale && Var0.Val.TruncBits == 0 &&
- Var0.Val.hasSameCastsAs(Var1.Val) && VisitedPhiBBs.empty() &&
+ Var0.Val.hasSameCastsAs(Var1.Val) && !MayBeCrossIteration &&
isKnownNonEqual(Var0.Val.V, Var1.Val.V, DL, &AC, /* CxtI */ nullptr,
DT))
MinAbsVarIndex = Var0.Scale.abs();
@@ -1432,19 +1433,14 @@ AliasResult BasicAAResult::aliasPHI(const PHINode *PN, LocationSize PNSize,
PNSize = LocationSize::beforeOrAfterPointer();
// In the recursive alias queries below, we may compare values from two
- //
diff erent loop iterations. Keep track of visited phi blocks, which will
- // be used when determining value equivalence.
- bool BlockInserted = VisitedPhiBBs.insert(PN->getParent()).second;
- auto _ = make_scope_exit([&]() {
- if (BlockInserted)
- VisitedPhiBBs.erase(PN->getParent());
- });
-
- // If we inserted a block into VisitedPhiBBs, alias analysis results that
+ //
diff erent loop iterations.
+ SaveAndRestore<bool> SavedMayBeCrossIteration(MayBeCrossIteration, true);
+
+ // If we enabled the MayBeCrossIteration flag, alias analysis results that
// have been cached earlier may no longer be valid. Perform recursive queries
// with a new AAQueryInfo.
AAQueryInfo NewAAQI = AAQI.withEmptyCache();
- AAQueryInfo *UseAAQI = BlockInserted ? &NewAAQI : &AAQI;
+ AAQueryInfo *UseAAQI = !SavedMayBeCrossIteration.get() ? &NewAAQI : &AAQI;
AliasResult Alias = AAQI.AAR.alias(MemoryLocation(V1Srcs[0], PNSize),
MemoryLocation(V2, V2Size), *UseAAQI);
@@ -1690,34 +1686,29 @@ AliasResult BasicAAResult::aliasCheckRecursive(
/// Check whether two Values can be considered equivalent.
///
-/// In addition to pointer equivalence of \p V1 and \p V2 this checks whether
-/// they can not be part of a cycle in the value graph by looking at all
-/// visited phi nodes an making sure that the phis cannot reach the value. We
-/// have to do this because we are looking through phi nodes (That is we say
+/// If the values may come from
diff erent cycle iterations, this will also
+/// check that the values are not part of cycle. We have to do this because we
+/// are looking through phi nodes, that is we say
/// noalias(V, phi(VA, VB)) if noalias(V, VA) and noalias(V, VB).
bool BasicAAResult::isValueEqualInPotentialCycles(const Value *V,
const Value *V2) {
if (V != V2)
return false;
- const Instruction *Inst = dyn_cast<Instruction>(V);
- if (!Inst)
+ if (!MayBeCrossIteration)
return true;
- if (VisitedPhiBBs.empty())
+ // Non-instructions and instructions in the entry block cannot be part of
+ // a loop.
+ const Instruction *Inst = dyn_cast<Instruction>(V);
+ if (!Inst || Inst->getParent()->isEntryBlock())
return true;
- if (VisitedPhiBBs.size() > MaxNumPhiBBsValueReachabilityCheck)
- return false;
-
- // Make sure that the visited phis cannot reach the Value. This ensures that
- // the Values cannot come from
diff erent iterations of a potential cycle the
- // phi nodes could be involved in.
- for (const auto *P : VisitedPhiBBs)
- if (isPotentiallyReachable(&P->front(), Inst, nullptr, DT))
- return false;
-
- return true;
+ // Check whether the instruction is part of a cycle, by checking whether the
+ // block can (non-trivially) reach itself.
+ BasicBlock *BB = const_cast<BasicBlock *>(Inst->getParent());
+ SmallVector<BasicBlock *> Succs(successors(BB));
+ return !isPotentiallyReachableFromMany(Succs, BB, nullptr, DT);
}
/// Computes the symbolic
diff erence between two de-composed GEPs.
More information about the llvm-commits
mailing list