[llvm] 2948242 - Revert "[LICM] Make promotion faster"
Alina Sbirlea via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 8 12:56:04 PST 2021
Author: Alina Sbirlea
Date: 2021-03-08T12:53:03-08:00
New Revision: 29482426b58e3c0e81ffe6b69c08067b3adc6461
URL: https://github.com/llvm/llvm-project/commit/29482426b58e3c0e81ffe6b69c08067b3adc6461
DIFF: https://github.com/llvm/llvm-project/commit/29482426b58e3c0e81ffe6b69c08067b3adc6461.diff
LOG: Revert "[LICM] Make promotion faster"
Revert 3d8f842712d49b0767832b6e3f65df2d3f19af4e
Revision triggers a miscompile sinking a store incorrectly outside a
threading loop. Detected by tsan.
Reverting while investigating.
Differential Revision: https://reviews.llvm.org/D89264
Added:
Modified:
llvm/include/llvm/Analysis/AliasSetTracker.h
llvm/lib/Analysis/AliasSetTracker.cpp
llvm/lib/Transforms/Scalar/LICM.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Analysis/AliasSetTracker.h b/llvm/include/llvm/Analysis/AliasSetTracker.h
index fe46afa3a537..b27fd5aa92a7 100644
--- a/llvm/include/llvm/Analysis/AliasSetTracker.h
+++ b/llvm/include/llvm/Analysis/AliasSetTracker.h
@@ -20,7 +20,6 @@
#include "llvm/ADT/DenseMapInfo.h"
#include "llvm/ADT/ilist.h"
#include "llvm/ADT/ilist_node.h"
-#include "llvm/Analysis/AliasAnalysis.h"
#include "llvm/Analysis/MemoryLocation.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/Metadata.h"
@@ -39,6 +38,8 @@ class AAResults;
class AliasSetTracker;
class BasicBlock;
class LoadInst;
+class Loop;
+class MemorySSA;
class AnyMemSetInst;
class AnyMemTransferInst;
class raw_ostream;
@@ -342,6 +343,8 @@ class AliasSetTracker {
struct ASTCallbackVHDenseMapInfo : public DenseMapInfo<Value *> {};
AAResults &AA;
+ MemorySSA *MSSA = nullptr;
+ Loop *L = nullptr;
ilist<AliasSet> AliasSets;
using PointerMapType = DenseMap<ASTCallbackVH, AliasSet::PointerRec *,
@@ -354,6 +357,8 @@ class AliasSetTracker {
/// Create an empty collection of AliasSets, and use the specified alias
/// analysis object to disambiguate load and store addresses.
explicit AliasSetTracker(AAResults &AA) : AA(AA) {}
+ explicit AliasSetTracker(AAResults &AA, MemorySSA *MSSA, Loop *L)
+ : AA(AA), MSSA(MSSA), L(L) {}
~AliasSetTracker() { clear(); }
/// These methods are used to add
diff erent types of instructions to the alias
@@ -378,6 +383,7 @@ class AliasSetTracker {
void add(BasicBlock &BB); // Add all instructions in basic block
void add(const AliasSetTracker &AST); // Add alias relations from another AST
void addUnknown(Instruction *I);
+ void addAllInstructionsInLoopUsingMSSA();
void clear();
diff --git a/llvm/lib/Analysis/AliasSetTracker.cpp b/llvm/lib/Analysis/AliasSetTracker.cpp
index 381433323ba8..8bd8a0763e91 100644
--- a/llvm/lib/Analysis/AliasSetTracker.cpp
+++ b/llvm/lib/Analysis/AliasSetTracker.cpp
@@ -14,6 +14,7 @@
#include "llvm/Analysis/GuardUtils.h"
#include "llvm/Analysis/LoopInfo.h"
#include "llvm/Analysis/MemoryLocation.h"
+#include "llvm/Analysis/MemorySSA.h"
#include "llvm/Config/llvm-config.h"
#include "llvm/IR/Constants.h"
#include "llvm/IR/DataLayout.h"
@@ -534,6 +535,15 @@ void AliasSetTracker::add(const AliasSetTracker &AST) {
}
}
+void AliasSetTracker::addAllInstructionsInLoopUsingMSSA() {
+ assert(MSSA && L && "MSSA and L must be available");
+ for (const BasicBlock *BB : L->blocks())
+ if (auto *Accesses = MSSA->getBlockAccesses(BB))
+ for (auto &Access : *Accesses)
+ if (auto *MUD = dyn_cast<MemoryUseOrDef>(&Access))
+ add(MUD->getMemoryInst());
+}
+
// deleteValue method - This method is used to remove a pointer value from the
// AliasSetTracker entirely. It should be used when an instruction is deleted
// from the program to update the AST. If you don't use this, you would have
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index 3bf3c2f0ad4d..d76f7a2ce9cc 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -185,12 +185,6 @@ static void moveInstructionBefore(Instruction &I, Instruction &Dest,
ICFLoopSafetyInfo &SafetyInfo,
MemorySSAUpdater *MSSAU, ScalarEvolution *SE);
-static void foreachMemoryAccess(MemorySSA *MSSA, Loop *L,
- function_ref<void(Instruction *)> Fn);
-static SmallVector<SmallSetVector<Value *, 8>, 0>
-collectPromotionCandidates(MemorySSA *MSSA, AliasAnalysis *AA, Loop *L,
- SmallVectorImpl<Instruction *> &MaybePromotable);
-
namespace {
struct LoopInvariantCodeMotion {
bool runOnLoop(Loop *L, AAResults *AA, LoopInfo *LI, DominatorTree *DT,
@@ -209,6 +203,9 @@ struct LoopInvariantCodeMotion {
std::unique_ptr<AliasSetTracker>
collectAliasInfoForLoop(Loop *L, LoopInfo *LI, AAResults *AA);
+ std::unique_ptr<AliasSetTracker>
+ collectAliasInfoForLoopWithMSSA(Loop *L, AAResults *AA,
+ MemorySSAUpdater *MSSAU);
};
struct LegacyLICMPass : public LoopPass {
@@ -449,48 +446,31 @@ bool LoopInvariantCodeMotion::runOnLoop(
PredIteratorCache PIC;
bool Promoted = false;
- if (CurAST.get()) {
- // Loop over all of the alias sets in the tracker object.
- for (AliasSet &AS : *CurAST) {
- // We can promote this alias set if it has a store, if it is a "Must"
- // alias set, if the pointer is loop invariant, and if we are not
- // eliminating any volatile loads or stores.
- if (AS.isForwardingAliasSet() || !AS.isMod() || !AS.isMustAlias() ||
- !L->isLoopInvariant(AS.begin()->getValue()))
- continue;
-
- assert(
- !AS.empty() &&
- "Must alias set should have at least one pointer element in it!");
-
- SmallSetVector<Value *, 8> PointerMustAliases;
- for (const auto &ASI : AS)
- PointerMustAliases.insert(ASI.getValue());
-
- Promoted |= promoteLoopAccessesToScalars(
- PointerMustAliases, ExitBlocks, InsertPts, MSSAInsertPts, PIC, LI,
- DT, TLI, L, CurAST.get(), MSSAU.get(), &SafetyInfo, ORE);
- }
- } else {
- SmallVector<Instruction *, 16> MaybePromotable;
- foreachMemoryAccess(MSSA, L, [&](Instruction *I) {
- MaybePromotable.push_back(I);
- });
-
- // Promoting one set of accesses may make the pointers for another set
- // loop invariant, so run this in a loop (with the MaybePromotable set
- // decreasing in size over time).
- bool LocalPromoted;
- do {
- LocalPromoted = false;
- for (const SmallSetVector<Value *, 8> &PointerMustAliases :
- collectPromotionCandidates(MSSA, AA, L, MaybePromotable)) {
- LocalPromoted |= promoteLoopAccessesToScalars(
- PointerMustAliases, ExitBlocks, InsertPts, MSSAInsertPts, PIC,
- LI, DT, TLI, L, /*AST*/nullptr, MSSAU.get(), &SafetyInfo, ORE);
- }
- Promoted |= LocalPromoted;
- } while (LocalPromoted);
+
+ // Build an AST using MSSA.
+ if (!CurAST.get())
+ CurAST = collectAliasInfoForLoopWithMSSA(L, AA, MSSAU.get());
+
+ // Loop over all of the alias sets in the tracker object.
+ for (AliasSet &AS : *CurAST) {
+ // We can promote this alias set if it has a store, if it is a "Must"
+ // alias set, if the pointer is loop invariant, and if we are not
+ // eliminating any volatile loads or stores.
+ if (AS.isForwardingAliasSet() || !AS.isMod() || !AS.isMustAlias() ||
+ !L->isLoopInvariant(AS.begin()->getValue()))
+ continue;
+
+ assert(
+ !AS.empty() &&
+ "Must alias set should have at least one pointer element in it!");
+
+ SmallSetVector<Value *, 8> PointerMustAliases;
+ for (const auto &ASI : AS)
+ PointerMustAliases.insert(ASI.getValue());
+
+ Promoted |= promoteLoopAccessesToScalars(
+ PointerMustAliases, ExitBlocks, InsertPts, MSSAInsertPts, PIC, LI,
+ DT, TLI, L, CurAST.get(), MSSAU.get(), &SafetyInfo, ORE);
}
// Once we have promoted values across the loop body we have to
@@ -2253,77 +2233,6 @@ bool llvm::promoteLoopAccessesToScalars(
return true;
}
-static void foreachMemoryAccess(MemorySSA *MSSA, Loop *L,
- function_ref<void(Instruction *)> Fn) {
- for (const BasicBlock *BB : L->blocks())
- if (const auto *Accesses = MSSA->getBlockAccesses(BB))
- for (const auto &Access : *Accesses)
- if (const auto *MUD = dyn_cast<MemoryUseOrDef>(&Access))
- Fn(MUD->getMemoryInst());
-}
-
-static SmallVector<SmallSetVector<Value *, 8>, 0>
-collectPromotionCandidates(MemorySSA *MSSA, AliasAnalysis *AA, Loop *L,
- SmallVectorImpl<Instruction *> &MaybePromotable) {
- AliasSetTracker AST(*AA);
-
- auto IsPotentiallyPromotable = [L](const Instruction *I) {
- if (const auto *SI = dyn_cast<StoreInst>(I))
- return L->isLoopInvariant(SI->getPointerOperand());
- if (const auto *LI = dyn_cast<LoadInst>(I))
- return L->isLoopInvariant(LI->getPointerOperand());
- return false;
- };
-
- // Populate AST with potentially promotable accesses and remove them from
- // MaybePromotable, so they will not be checked again on the next iteration.
- SmallPtrSet<Value *, 16> AttemptingPromotion;
- llvm::erase_if(MaybePromotable, [&](Instruction *I) {
- if (IsPotentiallyPromotable(I)) {
- AttemptingPromotion.insert(I);
- AST.add(I);
- return true;
- }
- return false;
- });
-
- // We're only interested in must-alias sets that contain a mod.
- SmallVector<const AliasSet *, 8> Sets;
- for (AliasSet &AS : AST)
- if (!AS.isForwardingAliasSet() && AS.isMod() && AS.isMustAlias())
- Sets.push_back(&AS);
-
- if (Sets.empty())
- return {}; // Nothing to promote...
-
- // Discard any sets for which there is an aliasing non-promotable access.
- foreachMemoryAccess(MSSA, L, [&](Instruction *I) {
- if (AttemptingPromotion.contains(I))
- return;
-
- if (Optional<MemoryLocation> Loc = MemoryLocation::getOrNone(I)) {
- llvm::erase_if(Sets, [&](const AliasSet *AS) {
- return AS->aliasesPointer(Loc->Ptr, Loc->Size, Loc->AATags, *AA)
- != NoAlias;
- });
- } else {
- llvm::erase_if(Sets, [&](const AliasSet *AS) {
- return AS->aliasesUnknownInst(I, *AA);
- });
- }
- });
-
- SmallVector<SmallSetVector<Value *, 8>, 0> Result;
- for (const AliasSet *Set : Sets) {
- SmallSetVector<Value *, 8> PointerMustAliases;
- for (const auto &ASI : *Set)
- PointerMustAliases.insert(ASI.getValue());
- Result.push_back(std::move(PointerMustAliases));
- }
-
- return Result;
-}
-
/// Returns an owning pointer to an alias set which incorporates aliasing info
/// from L and all subloops of L.
std::unique_ptr<AliasSetTracker>
@@ -2344,6 +2253,15 @@ LoopInvariantCodeMotion::collectAliasInfoForLoop(Loop *L, LoopInfo *LI,
return CurAST;
}
+std::unique_ptr<AliasSetTracker>
+LoopInvariantCodeMotion::collectAliasInfoForLoopWithMSSA(
+ Loop *L, AAResults *AA, MemorySSAUpdater *MSSAU) {
+ auto *MSSA = MSSAU->getMemorySSA();
+ auto CurAST = std::make_unique<AliasSetTracker>(*AA, MSSA, L);
+ CurAST->addAllInstructionsInLoopUsingMSSA();
+ return CurAST;
+}
+
static bool pointerInvalidatedByLoop(MemoryLocation MemLoc,
AliasSetTracker *CurAST, Loop *CurLoop,
AAResults *AA) {
More information about the llvm-commits
mailing list