From 3f1e8e0102b482973db88cfe4be92b80bc02fba3 Mon Sep 17 00:00:00 2001 From: Sanjoy Das Date: Sat, 11 Mar 2017 01:15:48 +0000 Subject: [PATCH] Use a WeakVH for UnknownInstructions in AliasSetTracker Summary: This change solves the same problem as D30726, except that this only throws out the bathwater. AST was not correctly tracking and deleting UnknownInstructions via handles. The existing code only tracks "pointers" in its `ASTCallbackVH`, so an UnknownInstruction (that isn't also def'ing a pointer used by another memory instruction) never gets a `ASTCallbackVH`. There are two other ways to solve this problem: - Use the `PointerRec` scheme for both known and unknown instructions. - Use a `CallbackVH` that erases the offending Instruction from the UnknownInstruction list. Both of the above changes seemed to be significantly (and unnecessarily IMO) more complex than this. Reviewers: chandlerc, dberlin, hfinkel, reames Subscribers: mcrosier, llvm-commits Differential Revision: https://reviews.llvm.org/D30849 llvm-svn: 297539 --- llvm/include/llvm/Analysis/AliasSetTracker.h | 7 ++-- llvm/lib/Analysis/AliasSetTracker.cpp | 36 ++++++++------------ llvm/test/Transforms/LICM/pr32129.ll | 18 ++++++++++ 3 files changed, 37 insertions(+), 24 deletions(-) create mode 100644 llvm/test/Transforms/LICM/pr32129.ll diff --git a/llvm/include/llvm/Analysis/AliasSetTracker.h b/llvm/include/llvm/Analysis/AliasSetTracker.h index 5d11b22c6eed..eac97501c759 100644 --- a/llvm/include/llvm/Analysis/AliasSetTracker.h +++ b/llvm/include/llvm/Analysis/AliasSetTracker.h @@ -121,7 +121,10 @@ class AliasSet : public ilist_node { AliasSet *Forward; /// All instructions without a specific address in this alias set. - std::vector > UnknownInsts; + /// In rare cases this vector can have a null'ed out WeakVH + /// instances (can happen if some other loop pass deletes an + /// instruction in this list). + std::vector UnknownInsts; /// Number of nodes pointing to this AliasSet plus the number of AliasSets /// forwarding to it. @@ -171,7 +174,7 @@ class AliasSet : public ilist_node { Instruction *getUnknownInst(unsigned i) const { assert(i < UnknownInsts.size()); - return UnknownInsts[i]; + return cast_or_null(UnknownInsts[i]); } public: diff --git a/llvm/lib/Analysis/AliasSetTracker.cpp b/llvm/lib/Analysis/AliasSetTracker.cpp index 701b0e1a5925..16b711a69ec3 100644 --- a/llvm/lib/Analysis/AliasSetTracker.cpp +++ b/llvm/lib/Analysis/AliasSetTracker.cpp @@ -199,9 +199,10 @@ bool AliasSet::aliasesPointer(const Value *Ptr, uint64_t Size, // Check the unknown instructions... if (!UnknownInsts.empty()) { for (unsigned i = 0, e = UnknownInsts.size(); i != e; ++i) - if (AA.getModRefInfo(UnknownInsts[i], - MemoryLocation(Ptr, Size, AAInfo)) != MRI_NoModRef) - return true; + if (auto *Inst = getUnknownInst(i)) + if (AA.getModRefInfo(Inst, MemoryLocation(Ptr, Size, AAInfo)) != + MRI_NoModRef) + return true; } return false; @@ -217,10 +218,12 @@ bool AliasSet::aliasesUnknownInst(const Instruction *Inst, return false; for (unsigned i = 0, e = UnknownInsts.size(); i != e; ++i) { - ImmutableCallSite C1(getUnknownInst(i)), C2(Inst); - if (!C1 || !C2 || AA.getModRefInfo(C1, C2) != MRI_NoModRef || - AA.getModRefInfo(C2, C1) != MRI_NoModRef) - return true; + if (auto *Inst = getUnknownInst(i)) { + ImmutableCallSite C1(Inst), C2(Inst); + if (!C1 || !C2 || AA.getModRefInfo(C1, C2) != MRI_NoModRef || + AA.getModRefInfo(C2, C1) != MRI_NoModRef) + return true; + } } for (iterator I = begin(), E = end(); I != E; ++I) @@ -471,7 +474,8 @@ void AliasSetTracker::add(const AliasSetTracker &AST) { // If there are any call sites in the alias set, add them to this AST. for (unsigned i = 0, e = AS.UnknownInsts.size(); i != e; ++i) - add(AS.UnknownInsts[i]); + if (auto *Inst = AS.getUnknownInst(i)) + add(Inst); // Loop over all of the pointers in this alias set. for (AliasSet::iterator ASI = AS.begin(), E = AS.end(); ASI != E; ++ASI) { @@ -489,19 +493,6 @@ void AliasSetTracker::add(const AliasSetTracker &AST) { // dangling pointers to deleted instructions. // void AliasSetTracker::deleteValue(Value *PtrVal) { - // If this is a call instruction, remove the callsite from the appropriate - // AliasSet (if present). - if (Instruction *Inst = dyn_cast(PtrVal)) { - if (Inst->mayReadOrWriteMemory()) { - // Scan all the alias sets to see if this call site is contained. - for (iterator I = begin(), E = end(); I != E;) { - iterator Cur = I++; - if (!Cur->Forward) - Cur->removeUnknownInst(*this, Inst); - } - } - } - // First, look up the PointerRec for this pointer. PointerMapType::iterator I = PointerMap.find_as(PtrVal); if (I == PointerMap.end()) return; // Noop @@ -633,7 +624,8 @@ void AliasSet::print(raw_ostream &OS) const { OS << "\n " << UnknownInsts.size() << " Unknown instructions: "; for (unsigned i = 0, e = UnknownInsts.size(); i != e; ++i) { if (i) OS << ", "; - UnknownInsts[i]->printAsOperand(OS); + if (auto *I = getUnknownInst(i)) + I->printAsOperand(OS); } } OS << "\n"; diff --git a/llvm/test/Transforms/LICM/pr32129.ll b/llvm/test/Transforms/LICM/pr32129.ll new file mode 100644 index 000000000000..2618afe46322 --- /dev/null +++ b/llvm/test/Transforms/LICM/pr32129.ll @@ -0,0 +1,18 @@ +; RUN: opt -S -licm -loop-unswitch -licm < %s | FileCheck %s + +declare void @llvm.experimental.guard(i1, ...) + +define void @test() { +; CHECK-LABEL: @test( +; CHECK-NOT: guard +entry: + br label %header + +header: + br label %loop + +loop: + %0 = icmp ult i32 0, 400 + call void (i1, ...) @llvm.experimental.guard(i1 %0, i32 9) [ "deopt"() ] + br i1 undef, label %header, label %loop +}