Skip to content

Commit 3ab9e8b

Browse files
author
Johannes Doerfert
committed
[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
1 parent 2d550d1 commit 3ab9e8b

File tree

15 files changed

+117
-72
lines changed

15 files changed

+117
-72
lines changed

‎llvm/include/llvm/Transforms/IPO/Attributor.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -733,6 +733,12 @@ struct Attributor {
733733
/// various places.
734734
void identifyDefaultAbstractAttributes(Function &F);
735735

736+
/// Initialize the information cache for queries regarding function \p F.
737+
///
738+
/// This method needs to be called for all function that might be looked at
739+
/// through the information cache interface *prior* to looking at them.
740+
void initializeInformationCache(Function &F);
741+
736742
/// Mark the internal function \p F as live.
737743
///
738744
/// This will trigger the identification and initialization of attributes for

‎llvm/lib/Transforms/IPO/Attributor.cpp

Lines changed: 98 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -1863,12 +1863,14 @@ struct AAIsDeadImpl : public AAIsDead {
18631863

18641864
void exploreFromEntry(Attributor &A, const Function *F) {
18651865
ToBeExploredPaths.insert(&(F->getEntryBlock().front()));
1866-
assumeLive(A, F->getEntryBlock());
18671866

18681867
for (size_t i = 0; i < ToBeExploredPaths.size(); ++i)
18691868
if (const Instruction *NextNoReturnI =
18701869
findNextNoReturn(A, ToBeExploredPaths[i]))
18711870
NoReturnCalls.insert(NextNoReturnI);
1871+
1872+
// Mark the block live after we looked for no-return instructions.
1873+
assumeLive(A, F->getEntryBlock());
18721874
}
18731875

18741876
/// Find the next assumed noreturn instruction in the block of \p I starting
@@ -3537,6 +3539,26 @@ bool Attributor::checkForAllReturnedValues(
35373539
});
35383540
}
35393541

3542+
static bool
3543+
checkForAllInstructionsImpl(InformationCache::OpcodeInstMapTy &OpcodeInstMap,
3544+
const function_ref<bool(Instruction &)> &Pred,
3545+
const AAIsDead *LivenessAA, bool &AnyDead,
3546+
const ArrayRef<unsigned> &Opcodes) {
3547+
for (unsigned Opcode : Opcodes) {
3548+
for (Instruction *I : OpcodeInstMap[Opcode]) {
3549+
// Skip dead instructions.
3550+
if (LivenessAA && LivenessAA->isAssumedDead(I)) {
3551+
AnyDead = true;
3552+
continue;
3553+
}
3554+
3555+
if (!Pred(*I))
3556+
return false;
3557+
}
3558+
}
3559+
return true;
3560+
}
3561+
35403562
bool Attributor::checkForAllInstructions(
35413563
const llvm::function_ref<bool(Instruction &)> &Pred,
35423564
const AbstractAttribute &QueryingAA, const ArrayRef<unsigned> &Opcodes) {
@@ -3555,18 +3577,8 @@ bool Attributor::checkForAllInstructions(
35553577

35563578
auto &OpcodeInstMap =
35573579
InfoCache.getOpcodeInstMapForFunction(*AssociatedFunction);
3558-
for (unsigned Opcode : Opcodes) {
3559-
for (Instruction *I : OpcodeInstMap[Opcode]) {
3560-
// Skip dead instructions.
3561-
if (LivenessAA.isAssumedDead(I)) {
3562-
AnyDead = true;
3563-
continue;
3564-
}
3565-
3566-
if (!Pred(*I))
3567-
return false;
3568-
}
3569-
}
3580+
if (!checkForAllInstructionsImpl(OpcodeInstMap, Pred, &LivenessAA, AnyDead, Opcodes))
3581+
return false;
35703582

35713583
// If we actually used liveness information so we have to record a dependence.
35723584
if (AnyDead)
@@ -3852,6 +3864,48 @@ ChangeStatus Attributor::run(Module &M) {
38523864
return ManifestChange;
38533865
}
38543866

3867+
void Attributor::initializeInformationCache(Function &F) {
3868+
3869+
// Walk all instructions to find interesting instructions that might be
3870+
// queried by abstract attributes during their initialization or update.
3871+
// This has to happen before we create attributes.
3872+
auto &ReadOrWriteInsts = InfoCache.FuncRWInstsMap[&F];
3873+
auto &InstOpcodeMap = InfoCache.FuncInstOpcodeMap[&F];
3874+
3875+
for (Instruction &I : instructions(&F)) {
3876+
bool IsInterestingOpcode = false;
3877+
3878+
// To allow easy access to all instructions in a function with a given
3879+
// opcode we store them in the InfoCache. As not all opcodes are interesting
3880+
// to concrete attributes we only cache the ones that are as identified in
3881+
// the following switch.
3882+
// Note: There are no concrete attributes now so this is initially empty.
3883+
switch (I.getOpcode()) {
3884+
default:
3885+
assert((!ImmutableCallSite(&I)) && (!isa<CallBase>(&I)) &&
3886+
"New call site/base instruction type needs to be known int the "
3887+
"Attributor.");
3888+
break;
3889+
case Instruction::Load:
3890+
// The alignment of a pointer is interesting for loads.
3891+
case Instruction::Store:
3892+
// The alignment of a pointer is interesting for stores.
3893+
case Instruction::Call:
3894+
case Instruction::CallBr:
3895+
case Instruction::Invoke:
3896+
case Instruction::CleanupRet:
3897+
case Instruction::CatchSwitch:
3898+
case Instruction::Resume:
3899+
case Instruction::Ret:
3900+
IsInterestingOpcode = true;
3901+
}
3902+
if (IsInterestingOpcode)
3903+
InstOpcodeMap[I.getOpcode()].push_back(&I);
3904+
if (I.mayReadOrWriteMemory())
3905+
ReadOrWriteInsts.push_back(&I);
3906+
}
3907+
}
3908+
38553909
void Attributor::identifyDefaultAbstractAttributes(Function &F) {
38563910
if (!VisitedFunctions.insert(&F).second)
38573911
return;
@@ -3935,52 +3989,9 @@ void Attributor::identifyDefaultAbstractAttributes(Function &F) {
39353989
}
39363990
}
39373991

3938-
// Walk all instructions to find more attribute opportunities and also
3939-
// interesting instructions that might be queried by abstract attributes
3940-
// during their initialization or update.
3941-
auto &ReadOrWriteInsts = InfoCache.FuncRWInstsMap[&F];
3942-
auto &InstOpcodeMap = InfoCache.FuncInstOpcodeMap[&F];
3943-
3944-
for (Instruction &I : instructions(&F)) {
3945-
bool IsInterestingOpcode = false;
3946-
3947-
// To allow easy access to all instructions in a function with a given
3948-
// opcode we store them in the InfoCache. As not all opcodes are interesting
3949-
// to concrete attributes we only cache the ones that are as identified in
3950-
// the following switch.
3951-
// Note: There are no concrete attributes now so this is initially empty.
3952-
switch (I.getOpcode()) {
3953-
default:
3954-
assert((!ImmutableCallSite(&I)) && (!isa<CallBase>(&I)) &&
3955-
"New call site/base instruction type needs to be known int the "
3956-
"attributor.");
3957-
break;
3958-
case Instruction::Load:
3959-
// The alignment of a pointer is interesting for loads.
3960-
getOrCreateAAFor<AAAlign>(
3961-
IRPosition::value(*cast<LoadInst>(I).getPointerOperand()));
3962-
break;
3963-
case Instruction::Store:
3964-
// The alignment of a pointer is interesting for stores.
3965-
getOrCreateAAFor<AAAlign>(
3966-
IRPosition::value(*cast<StoreInst>(I).getPointerOperand()));
3967-
break;
3968-
case Instruction::Call:
3969-
case Instruction::CallBr:
3970-
case Instruction::Invoke:
3971-
case Instruction::CleanupRet:
3972-
case Instruction::CatchSwitch:
3973-
case Instruction::Resume:
3974-
case Instruction::Ret:
3975-
IsInterestingOpcode = true;
3976-
}
3977-
if (IsInterestingOpcode)
3978-
InstOpcodeMap[I.getOpcode()].push_back(&I);
3979-
if (I.mayReadOrWriteMemory())
3980-
ReadOrWriteInsts.push_back(&I);
3981-
3992+
auto CallSitePred = [&](Instruction &I) -> bool {
39823993
CallSite CS(&I);
3983-
if (CS && CS.getCalledFunction()) {
3994+
if (CS.getCalledFunction()) {
39843995
for (int i = 0, e = CS.getCalledFunction()->arg_size(); i < e; i++) {
39853996

39863997
IRPosition CSArgPos = IRPosition::callsite_argument(CS, i);
@@ -4004,7 +4015,32 @@ void Attributor::identifyDefaultAbstractAttributes(Function &F) {
40044015
getOrCreateAAFor<AAAlign>(CSArgPos);
40054016
}
40064017
}
4007-
}
4018+
return true;
4019+
};
4020+
4021+
auto &OpcodeInstMap = InfoCache.getOpcodeInstMapForFunction(F);
4022+
bool Success, AnyDead = false;
4023+
Success = checkForAllInstructionsImpl(
4024+
OpcodeInstMap, CallSitePred, nullptr, AnyDead,
4025+
{(unsigned)Instruction::Invoke, (unsigned)Instruction::CallBr,
4026+
(unsigned)Instruction::Call});
4027+
(void)Success;
4028+
assert(Success && !AnyDead && "Expected the check call to be successful!");
4029+
4030+
auto LoadStorePred = [&](Instruction &I) -> bool {
4031+
if (isa<LoadInst>(I))
4032+
getOrCreateAAFor<AAAlign>(
4033+
IRPosition::value(*cast<LoadInst>(I).getPointerOperand()));
4034+
else
4035+
getOrCreateAAFor<AAAlign>(
4036+
IRPosition::value(*cast<StoreInst>(I).getPointerOperand()));
4037+
return true;
4038+
};
4039+
Success = checkForAllInstructionsImpl(
4040+
OpcodeInstMap, LoadStorePred, nullptr, AnyDead,
4041+
{(unsigned)Instruction::Load, (unsigned)Instruction::Store});
4042+
(void)Success;
4043+
assert(Success && !AnyDead && "Expected the check call to be successful!");
40084044
}
40094045

40104046
/// Helpers to ease debugging through output streams and print calls.
@@ -4078,6 +4114,9 @@ static bool runAttributorOnModule(Module &M, AnalysisGetter &AG) {
40784114
InformationCache InfoCache(M.getDataLayout(), AG);
40794115
Attributor A(InfoCache, DepRecInterval);
40804116

4117+
for (Function &F : M)
4118+
A.initializeInformationCache(F);
4119+
40814120
for (Function &F : M) {
40824121
if (F.hasExactDefinition())
40834122
NumFnWithExactDefinition++;

‎llvm/test/Transforms/FunctionAttrs/align.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=7 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
1+
; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=6 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
22

33
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
44

‎llvm/test/Transforms/FunctionAttrs/arg_returned.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
; RUN: opt -functionattrs -S < %s | FileCheck %s --check-prefix=FNATTR
2-
; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=9 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
2+
; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=6 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
33
; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -functionattrs -S < %s | FileCheck %s --check-prefix=BOTH
44
;
55
; Test cases specifically designed for the "returned" argument attribute.

‎llvm/test/Transforms/FunctionAttrs/dereferenceable.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
; RUN: opt -attributor -attributor-manifest-internal --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s --check-prefixes=ATTRIBUTOR
1+
; RUN: opt -attributor -attributor-manifest-internal --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=1 -S < %s | FileCheck %s --check-prefixes=ATTRIBUTOR
22

33

44
declare void @deref_phi_user(i32* %a);

‎llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 -S < %s | FileCheck %s
1+
; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s
22
;
33
; Test cases specifically designed for the "no-return" function attribute.
44
; We use FIXME's to indicate problems and missing attributes.

‎llvm/test/Transforms/FunctionAttrs/internal-noalias.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=6 < %s | FileCheck %s
1+
; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=5 < %s | FileCheck %s
22

33
define dso_local i32 @visible(i32* noalias %A, i32* noalias %B) #0 {
44
entry:

‎llvm/test/Transforms/FunctionAttrs/liveness.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=11 -S < %s | FileCheck %s
1+
; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s
22

33
declare void @no_return_call() nofree noreturn nounwind readnone
44

‎llvm/test/Transforms/FunctionAttrs/noalias_returned.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=5 < %s | FileCheck %s
1+
; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=1 < %s | FileCheck %s
22

33
; TEST 1 - negative.
44

‎llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
; RUN: opt -functionattrs --disable-nofree-inference=false -S < %s | FileCheck %s --check-prefix=FNATTR
2-
; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=7 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
2+
; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=4 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
33

44
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
55

‎llvm/test/Transforms/FunctionAttrs/noreturn_async.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 -S < %s | FileCheck %s
1+
; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s
22
;
33
; This file is the same as noreturn_sync.ll but with a personality which
44
; indicates that the exception handler *can* catch asynchronous exceptions. As

‎llvm/test/Transforms/FunctionAttrs/noreturn_sync.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=5 -S < %s | FileCheck %s
1+
; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s
22
;
33
; This file is the same as noreturn_async.ll but with a personality which
44
; indicates that the exception handler *cannot* catch asynchronous exceptions.

‎llvm/test/Transforms/FunctionAttrs/nosync.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
; RUN: opt -functionattrs -S < %s | FileCheck %s --check-prefix=FNATTR
2-
; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
2+
; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
33
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
44

55
; Test cases designed for the nosync function attribute.

‎llvm/test/Transforms/FunctionAttrs/nounwind.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
; RUN: opt < %s -functionattrs -S | FileCheck %s
2-
; RUN: opt < %s -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=5 -S | FileCheck %s --check-prefix=ATTRIBUTOR
2+
; RUN: opt < %s -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S | FileCheck %s --check-prefix=ATTRIBUTOR
33

44
; TEST 1
55
; CHECK: Function Attrs: norecurse nounwind readnone

‎llvm/test/Transforms/FunctionAttrs/willreturn.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
; RUN: opt -functionattrs -S < %s | FileCheck %s --check-prefix=FNATTR
2-
; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=7 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
2+
; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
33

44

55
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

0 commit comments

Comments
 (0)