From 3ab9e8b81858cdcf4f381b4238315cb1d434e984 Mon Sep 17 00:00:00 2001 From: Johannes Doerfert Date: Tue, 17 Sep 2019 10:52:41 +0000 Subject: [PATCH] [Attributor][Fix] Initialize the cache prior to using it Summary: There were segfaults as we modified and iterated the instruction maps in the cache at the same time. This was happening because we created new instructions while we populated the cache. This fix changes the order in which we perform these actions. First, the caches for the whole module are created, then we start to create abstract attributes. I don't have a unit test but the LLVM test suite exposes this problem. Reviewers: uenoku, sstefan1 Subscribers: hiraditya, bollu, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D67232 llvm-svn: 372105 --- llvm/include/llvm/Transforms/IPO/Attributor.h | 6 + llvm/lib/Transforms/IPO/Attributor.cpp | 157 +++++++++++------- llvm/test/Transforms/FunctionAttrs/align.ll | 2 +- .../Transforms/FunctionAttrs/arg_returned.ll | 2 +- .../FunctionAttrs/dereferenceable.ll | 2 +- .../Transforms/FunctionAttrs/fn_noreturn.ll | 2 +- .../FunctionAttrs/internal-noalias.ll | 2 +- .../test/Transforms/FunctionAttrs/liveness.ll | 2 +- .../FunctionAttrs/noalias_returned.ll | 2 +- .../FunctionAttrs/nofree-attributor.ll | 2 +- .../FunctionAttrs/noreturn_async.ll | 2 +- .../Transforms/FunctionAttrs/noreturn_sync.ll | 2 +- llvm/test/Transforms/FunctionAttrs/nosync.ll | 2 +- .../test/Transforms/FunctionAttrs/nounwind.ll | 2 +- .../Transforms/FunctionAttrs/willreturn.ll | 2 +- 15 files changed, 117 insertions(+), 72 deletions(-) diff --git a/llvm/include/llvm/Transforms/IPO/Attributor.h b/llvm/include/llvm/Transforms/IPO/Attributor.h index 95cc07d31b34..3be021bc3364 100644 --- a/llvm/include/llvm/Transforms/IPO/Attributor.h +++ b/llvm/include/llvm/Transforms/IPO/Attributor.h @@ -733,6 +733,12 @@ struct Attributor { /// various places. void identifyDefaultAbstractAttributes(Function &F); + /// Initialize the information cache for queries regarding function \p F. + /// + /// This method needs to be called for all function that might be looked at + /// through the information cache interface *prior* to looking at them. + void initializeInformationCache(Function &F); + /// Mark the internal function \p F as live. /// /// This will trigger the identification and initialization of attributes for diff --git a/llvm/lib/Transforms/IPO/Attributor.cpp b/llvm/lib/Transforms/IPO/Attributor.cpp index a6f8472e85db..654f2084be4b 100644 --- a/llvm/lib/Transforms/IPO/Attributor.cpp +++ b/llvm/lib/Transforms/IPO/Attributor.cpp @@ -1863,12 +1863,14 @@ struct AAIsDeadImpl : public AAIsDead { void exploreFromEntry(Attributor &A, const Function *F) { ToBeExploredPaths.insert(&(F->getEntryBlock().front())); - assumeLive(A, F->getEntryBlock()); for (size_t i = 0; i < ToBeExploredPaths.size(); ++i) if (const Instruction *NextNoReturnI = findNextNoReturn(A, ToBeExploredPaths[i])) NoReturnCalls.insert(NextNoReturnI); + + // Mark the block live after we looked for no-return instructions. + assumeLive(A, F->getEntryBlock()); } /// Find the next assumed noreturn instruction in the block of \p I starting @@ -3537,6 +3539,26 @@ bool Attributor::checkForAllReturnedValues( }); } +static bool +checkForAllInstructionsImpl(InformationCache::OpcodeInstMapTy &OpcodeInstMap, + const function_ref &Pred, + const AAIsDead *LivenessAA, bool &AnyDead, + const ArrayRef &Opcodes) { + for (unsigned Opcode : Opcodes) { + for (Instruction *I : OpcodeInstMap[Opcode]) { + // Skip dead instructions. + if (LivenessAA && LivenessAA->isAssumedDead(I)) { + AnyDead = true; + continue; + } + + if (!Pred(*I)) + return false; + } + } + return true; +} + bool Attributor::checkForAllInstructions( const llvm::function_ref &Pred, const AbstractAttribute &QueryingAA, const ArrayRef &Opcodes) { @@ -3555,18 +3577,8 @@ bool Attributor::checkForAllInstructions( auto &OpcodeInstMap = InfoCache.getOpcodeInstMapForFunction(*AssociatedFunction); - for (unsigned Opcode : Opcodes) { - for (Instruction *I : OpcodeInstMap[Opcode]) { - // Skip dead instructions. - if (LivenessAA.isAssumedDead(I)) { - AnyDead = true; - continue; - } - - if (!Pred(*I)) - return false; - } - } + if (!checkForAllInstructionsImpl(OpcodeInstMap, Pred, &LivenessAA, AnyDead, Opcodes)) + return false; // If we actually used liveness information so we have to record a dependence. if (AnyDead) @@ -3852,6 +3864,48 @@ ChangeStatus Attributor::run(Module &M) { return ManifestChange; } +void Attributor::initializeInformationCache(Function &F) { + + // Walk all instructions to find interesting instructions that might be + // queried by abstract attributes during their initialization or update. + // This has to happen before we create attributes. + auto &ReadOrWriteInsts = InfoCache.FuncRWInstsMap[&F]; + auto &InstOpcodeMap = InfoCache.FuncInstOpcodeMap[&F]; + + for (Instruction &I : instructions(&F)) { + bool IsInterestingOpcode = false; + + // To allow easy access to all instructions in a function with a given + // opcode we store them in the InfoCache. As not all opcodes are interesting + // to concrete attributes we only cache the ones that are as identified in + // the following switch. + // Note: There are no concrete attributes now so this is initially empty. + switch (I.getOpcode()) { + default: + assert((!ImmutableCallSite(&I)) && (!isa(&I)) && + "New call site/base instruction type needs to be known int the " + "Attributor."); + break; + case Instruction::Load: + // The alignment of a pointer is interesting for loads. + case Instruction::Store: + // The alignment of a pointer is interesting for stores. + case Instruction::Call: + case Instruction::CallBr: + case Instruction::Invoke: + case Instruction::CleanupRet: + case Instruction::CatchSwitch: + case Instruction::Resume: + case Instruction::Ret: + IsInterestingOpcode = true; + } + if (IsInterestingOpcode) + InstOpcodeMap[I.getOpcode()].push_back(&I); + if (I.mayReadOrWriteMemory()) + ReadOrWriteInsts.push_back(&I); + } +} + void Attributor::identifyDefaultAbstractAttributes(Function &F) { if (!VisitedFunctions.insert(&F).second) return; @@ -3935,52 +3989,9 @@ void Attributor::identifyDefaultAbstractAttributes(Function &F) { } } - // Walk all instructions to find more attribute opportunities and also - // interesting instructions that might be queried by abstract attributes - // during their initialization or update. - auto &ReadOrWriteInsts = InfoCache.FuncRWInstsMap[&F]; - auto &InstOpcodeMap = InfoCache.FuncInstOpcodeMap[&F]; - - for (Instruction &I : instructions(&F)) { - bool IsInterestingOpcode = false; - - // To allow easy access to all instructions in a function with a given - // opcode we store them in the InfoCache. As not all opcodes are interesting - // to concrete attributes we only cache the ones that are as identified in - // the following switch. - // Note: There are no concrete attributes now so this is initially empty. - switch (I.getOpcode()) { - default: - assert((!ImmutableCallSite(&I)) && (!isa(&I)) && - "New call site/base instruction type needs to be known int the " - "attributor."); - break; - case Instruction::Load: - // The alignment of a pointer is interesting for loads. - getOrCreateAAFor( - IRPosition::value(*cast(I).getPointerOperand())); - break; - case Instruction::Store: - // The alignment of a pointer is interesting for stores. - getOrCreateAAFor( - IRPosition::value(*cast(I).getPointerOperand())); - break; - case Instruction::Call: - case Instruction::CallBr: - case Instruction::Invoke: - case Instruction::CleanupRet: - case Instruction::CatchSwitch: - case Instruction::Resume: - case Instruction::Ret: - IsInterestingOpcode = true; - } - if (IsInterestingOpcode) - InstOpcodeMap[I.getOpcode()].push_back(&I); - if (I.mayReadOrWriteMemory()) - ReadOrWriteInsts.push_back(&I); - + auto CallSitePred = [&](Instruction &I) -> bool { CallSite CS(&I); - if (CS && CS.getCalledFunction()) { + if (CS.getCalledFunction()) { for (int i = 0, e = CS.getCalledFunction()->arg_size(); i < e; i++) { IRPosition CSArgPos = IRPosition::callsite_argument(CS, i); @@ -4004,7 +4015,32 @@ void Attributor::identifyDefaultAbstractAttributes(Function &F) { getOrCreateAAFor(CSArgPos); } } - } + return true; + }; + + auto &OpcodeInstMap = InfoCache.getOpcodeInstMapForFunction(F); + bool Success, AnyDead = false; + Success = checkForAllInstructionsImpl( + OpcodeInstMap, CallSitePred, nullptr, AnyDead, + {(unsigned)Instruction::Invoke, (unsigned)Instruction::CallBr, + (unsigned)Instruction::Call}); + (void)Success; + assert(Success && !AnyDead && "Expected the check call to be successful!"); + + auto LoadStorePred = [&](Instruction &I) -> bool { + if (isa(I)) + getOrCreateAAFor( + IRPosition::value(*cast(I).getPointerOperand())); + else + getOrCreateAAFor( + IRPosition::value(*cast(I).getPointerOperand())); + return true; + }; + Success = checkForAllInstructionsImpl( + OpcodeInstMap, LoadStorePred, nullptr, AnyDead, + {(unsigned)Instruction::Load, (unsigned)Instruction::Store}); + (void)Success; + assert(Success && !AnyDead && "Expected the check call to be successful!"); } /// Helpers to ease debugging through output streams and print calls. @@ -4078,6 +4114,9 @@ static bool runAttributorOnModule(Module &M, AnalysisGetter &AG) { InformationCache InfoCache(M.getDataLayout(), AG); Attributor A(InfoCache, DepRecInterval); + for (Function &F : M) + A.initializeInformationCache(F); + for (Function &F : M) { if (F.hasExactDefinition()) NumFnWithExactDefinition++; diff --git a/llvm/test/Transforms/FunctionAttrs/align.ll b/llvm/test/Transforms/FunctionAttrs/align.ll index 5e8671b64a2e..fc5ffc61e8ea 100644 --- a/llvm/test/Transforms/FunctionAttrs/align.ll +++ b/llvm/test/Transforms/FunctionAttrs/align.ll @@ -1,4 +1,4 @@ -; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=7 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR +; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=6 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" diff --git a/llvm/test/Transforms/FunctionAttrs/arg_returned.ll b/llvm/test/Transforms/FunctionAttrs/arg_returned.ll index 95871ff7d34e..57d2713d278b 100644 --- a/llvm/test/Transforms/FunctionAttrs/arg_returned.ll +++ b/llvm/test/Transforms/FunctionAttrs/arg_returned.ll @@ -1,5 +1,5 @@ ; RUN: opt -functionattrs -S < %s | FileCheck %s --check-prefix=FNATTR -; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=9 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR +; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=6 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR ; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -functionattrs -S < %s | FileCheck %s --check-prefix=BOTH ; ; Test cases specifically designed for the "returned" argument attribute. diff --git a/llvm/test/Transforms/FunctionAttrs/dereferenceable.ll b/llvm/test/Transforms/FunctionAttrs/dereferenceable.ll index 852b2a283517..d7b576f506ac 100644 --- a/llvm/test/Transforms/FunctionAttrs/dereferenceable.ll +++ b/llvm/test/Transforms/FunctionAttrs/dereferenceable.ll @@ -1,4 +1,4 @@ -; RUN: opt -attributor -attributor-manifest-internal --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s --check-prefixes=ATTRIBUTOR +; RUN: opt -attributor -attributor-manifest-internal --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=1 -S < %s | FileCheck %s --check-prefixes=ATTRIBUTOR declare void @deref_phi_user(i32* %a); diff --git a/llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll b/llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll index d8258be7d2cc..e3a0f0b2504c 100644 --- a/llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll +++ b/llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll @@ -1,4 +1,4 @@ -; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 -S < %s | FileCheck %s +; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s ; ; Test cases specifically designed for the "no-return" function attribute. ; We use FIXME's to indicate problems and missing attributes. diff --git a/llvm/test/Transforms/FunctionAttrs/internal-noalias.ll b/llvm/test/Transforms/FunctionAttrs/internal-noalias.ll index edc3fe07dae9..1118fa194f3c 100644 --- a/llvm/test/Transforms/FunctionAttrs/internal-noalias.ll +++ b/llvm/test/Transforms/FunctionAttrs/internal-noalias.ll @@ -1,4 +1,4 @@ -; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=6 < %s | FileCheck %s +; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=5 < %s | FileCheck %s define dso_local i32 @visible(i32* noalias %A, i32* noalias %B) #0 { entry: diff --git a/llvm/test/Transforms/FunctionAttrs/liveness.ll b/llvm/test/Transforms/FunctionAttrs/liveness.ll index 93aac9999507..771c99309bb1 100644 --- a/llvm/test/Transforms/FunctionAttrs/liveness.ll +++ b/llvm/test/Transforms/FunctionAttrs/liveness.ll @@ -1,4 +1,4 @@ -; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=11 -S < %s | FileCheck %s +; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s declare void @no_return_call() nofree noreturn nounwind readnone diff --git a/llvm/test/Transforms/FunctionAttrs/noalias_returned.ll b/llvm/test/Transforms/FunctionAttrs/noalias_returned.ll index 410d1d6cbc35..7174c3dc97cb 100644 --- a/llvm/test/Transforms/FunctionAttrs/noalias_returned.ll +++ b/llvm/test/Transforms/FunctionAttrs/noalias_returned.ll @@ -1,4 +1,4 @@ -; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=5 < %s | FileCheck %s +; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=1 < %s | FileCheck %s ; TEST 1 - negative. diff --git a/llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll b/llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll index bfaa98eccf75..f76b429aa4b3 100644 --- a/llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll +++ b/llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll @@ -1,5 +1,5 @@ ; RUN: opt -functionattrs --disable-nofree-inference=false -S < %s | FileCheck %s --check-prefix=FNATTR -; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=7 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR +; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=4 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" diff --git a/llvm/test/Transforms/FunctionAttrs/noreturn_async.ll b/llvm/test/Transforms/FunctionAttrs/noreturn_async.ll index 1e34ca4f9486..e9e423688df9 100644 --- a/llvm/test/Transforms/FunctionAttrs/noreturn_async.ll +++ b/llvm/test/Transforms/FunctionAttrs/noreturn_async.ll @@ -1,4 +1,4 @@ -; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 -S < %s | FileCheck %s +; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s ; ; This file is the same as noreturn_sync.ll but with a personality which ; indicates that the exception handler *can* catch asynchronous exceptions. As diff --git a/llvm/test/Transforms/FunctionAttrs/noreturn_sync.ll b/llvm/test/Transforms/FunctionAttrs/noreturn_sync.ll index 4c62ea57f509..5d644965fb27 100644 --- a/llvm/test/Transforms/FunctionAttrs/noreturn_sync.ll +++ b/llvm/test/Transforms/FunctionAttrs/noreturn_sync.ll @@ -1,4 +1,4 @@ -; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=5 -S < %s | FileCheck %s +; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s ; ; This file is the same as noreturn_async.ll but with a personality which ; indicates that the exception handler *cannot* catch asynchronous exceptions. diff --git a/llvm/test/Transforms/FunctionAttrs/nosync.ll b/llvm/test/Transforms/FunctionAttrs/nosync.ll index 2807e89e9861..d948048db129 100644 --- a/llvm/test/Transforms/FunctionAttrs/nosync.ll +++ b/llvm/test/Transforms/FunctionAttrs/nosync.ll @@ -1,5 +1,5 @@ ; RUN: opt -functionattrs -S < %s | FileCheck %s --check-prefix=FNATTR -; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR +; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" ; Test cases designed for the nosync function attribute. diff --git a/llvm/test/Transforms/FunctionAttrs/nounwind.ll b/llvm/test/Transforms/FunctionAttrs/nounwind.ll index 0e6b607d5461..858780589228 100644 --- a/llvm/test/Transforms/FunctionAttrs/nounwind.ll +++ b/llvm/test/Transforms/FunctionAttrs/nounwind.ll @@ -1,5 +1,5 @@ ; RUN: opt < %s -functionattrs -S | FileCheck %s -; RUN: opt < %s -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=5 -S | FileCheck %s --check-prefix=ATTRIBUTOR +; RUN: opt < %s -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S | FileCheck %s --check-prefix=ATTRIBUTOR ; TEST 1 ; CHECK: Function Attrs: norecurse nounwind readnone diff --git a/llvm/test/Transforms/FunctionAttrs/willreturn.ll b/llvm/test/Transforms/FunctionAttrs/willreturn.ll index 1b9b3c91b31b..7821658c9529 100644 --- a/llvm/test/Transforms/FunctionAttrs/willreturn.ll +++ b/llvm/test/Transforms/FunctionAttrs/willreturn.ll @@ -1,5 +1,5 @@ ; RUN: opt -functionattrs -S < %s | FileCheck %s --check-prefix=FNATTR -; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=7 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR +; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" -- 2.34.1