Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FunctionAttrs] Add the "initializes" attribute inference #97373

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
334 changes: 331 additions & 3 deletions llvm/lib/Transforms/IPO/FunctionAttrs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "llvm/Transforms/IPO/FunctionAttrs.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/PostOrderIterator.h"
#include "llvm/ADT/SCCIterator.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SetVector.h"
Expand All @@ -36,6 +37,7 @@
#include "llvm/IR/Attributes.h"
#include "llvm/IR/BasicBlock.h"
#include "llvm/IR/Constant.h"
#include "llvm/IR/ConstantRangeList.h"
#include "llvm/IR/Constants.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/InstIterator.h"
Expand Down Expand Up @@ -580,6 +582,205 @@ struct ArgumentUsesTracker : public CaptureTracker {
const SCCNodeSet &SCCNodes;
};

struct ArgumentUse {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reminding! Done.

Use *U;
std::optional<int64_t> Offset;
};

// A struct of argument access info. "Unknown" accesses are the cases like
// unrecognized instructions, instructions that have more than one use of
// the argument, or volatile memory accesses. "Unknown" implies "IsClobber"
// and an empty access range.
// Write or Read accesses can be clobbers as well for example, a Load with
// scalable type.
struct ArgumentAccessInfo {
enum AccessType { Write, Read, Unknown };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enum class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

AccessType ArgAccessType;
ConstantRangeList AccessRanges;
bool IsClobber = false;
jvoung marked this conversation as resolved.
Show resolved Hide resolved
};

struct UsesPerBlockInfo {
DenseMap<Instruction *, ArgumentAccessInfo> Insts;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How large is Insts typically?

DenseMap jumps from 0 to 64: https://llvm.org/docs/ProgrammersManual.html#llvm-adt-densemap-h

SmallDenseMap<..., 8> or something might help make it more gradual 0 -> 8 -> 64

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! According to an internal benchmark (muppet), 90% have a <= 2 size. Changed to SmallDenseMap<..., 2>.

Similarly to UsesPerBlock, 90% have a <=8 size so changed UsesPerBlock to SmallDenseMap<..., 8>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's <= 2, and a non-trivial amount are == 2, you may want to provide 3 inline buckets to avoid having to grow() when the 2nd entry is inserted.
There's a max load factor of 3/4

// If the load of the hash table is more than 3/4, or if fewer than 1/8 of
so I think it won't allow filling 2 inline buckets with 2 entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good point! Changed the SmallDenseMap size here to 4 and the size of UsesPerBlock to 16.
With 4*3/4 (<=3) and 16*3/4 (<=12), they covers 96% and 94% cases in an internal large benchmark. Thanks!

BTW, the size must be a power of 2 so choose 4 and 16.

static_assert(isPowerOf2_64(InlineBuckets),

bool HasWrites;
bool HasClobber;
};

ArgumentAccessInfo GetArgmentAccessInfo(const Instruction *I,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ArgumentAccessInfo GetArgmentAccessInfo(const Instruction *I,
ArgumentAccessInfo getArgmentAccessInfo(const Instruction *I,

const ArgumentUse &IU,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Would it make more sense to name the variable "ArgUse"? Unclear to me why "I" in the name?

Similar below "ArgumentUse IU = ..." at line 756

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, legacy names. Changed to "ArgUse".

const DataLayout &DL) {
auto GetTypeAccessRange =
[&DL](Type *Ty,
std::optional<int64_t> Offset) -> std::optional<ConstantRange> {
auto TypeSize = DL.getTypeStoreSize(Ty);
if (!TypeSize.isScalable() && Offset.has_value()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in many if-statements, you write optional.has_value() . optional is sufficient and the preferred form.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!

int64_t Size = TypeSize.getFixedValue();
return ConstantRange(APInt(64, Offset.value(), true),
APInt(64, Offset.value() + Size, true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To access the value of optionals, you often write optional.value(). The preferred from is *optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

}
return std::nullopt;
};
auto GetConstantIntRange =
[](Value *Length,
std::optional<int64_t> Offset) -> std::optional<ConstantRange> {
auto *ConstantLength = dyn_cast<ConstantInt>(Length);
if (ConstantLength && Offset.has_value()) {
return ConstantRange(
APInt(64, Offset.value(), true),
APInt(64, Offset.value() + ConstantLength->getSExtValue(), true));
}
return std::nullopt;
};
if (auto *SI = dyn_cast<StoreInst>(I)) {
aeubanks marked this conversation as resolved.
Show resolved Hide resolved
if (&SI->getOperandUse(1) == IU.U) {
// Get the fixed type size of "SI". Since the access range of a write
// will be unioned, if "SI" doesn't have a fixed type size, we just set
// the access range to empty.
ConstantRangeList AccessRanges;
auto TypeAccessRange = GetTypeAccessRange(SI->getAccessType(), IU.Offset);
if (TypeAccessRange.has_value())
AccessRanges.insert(TypeAccessRange.value());
return {ArgumentAccessInfo::AccessType::Write, AccessRanges,
/*IsClobber=*/false};
}
} else if (auto *LI = dyn_cast<LoadInst>(I)) {
if (&LI->getOperandUse(0) == IU.U) {
// Get the fixed type size of "LI". Different from Write, if "LI"
// doesn't have a fixed type size, we conservatively set as a clobber
// with an empty access range.
auto TypeAccessRange = GetTypeAccessRange(LI->getAccessType(), IU.Offset);
if (TypeAccessRange.has_value())
return {ArgumentAccessInfo::AccessType::Read,
{TypeAccessRange.value()},
/*IsClobber=*/false};
else
return {ArgumentAccessInfo::AccessType::Read, {}, /*IsClobber=*/true};
}
} else if (auto *MemSet = dyn_cast<MemSetInst>(I)) {
if (!MemSet->isVolatile()) {
ConstantRangeList AccessRanges;
auto AccessRange = GetConstantIntRange(MemSet->getLength(), IU.Offset);
if (AccessRange.has_value())
AccessRanges.insert(AccessRange.value());
return {ArgumentAccessInfo::AccessType::Write, AccessRanges,
/*IsClobber=*/false};
}
} else if (auto *MemCpy = dyn_cast<MemCpyInst>(I)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be MemTransferInst to cover memmove too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point! Changed to MemTransferInst and added unit test for memmove as well.

if (!MemCpy->isVolatile()) {
if (&MemCpy->getOperandUse(0) == IU.U) {
ConstantRangeList AccessRanges;
auto AccessRange = GetConstantIntRange(MemCpy->getLength(), IU.Offset);
if (AccessRange.has_value())
AccessRanges.insert(AccessRange.value());
return {ArgumentAccessInfo::AccessType::Write, AccessRanges,
/*IsClobber=*/false};
} else if (&MemCpy->getOperandUse(1) == IU.U) {
auto AccessRange = GetConstantIntRange(MemCpy->getLength(), IU.Offset);
if (AccessRange.has_value())
return {ArgumentAccessInfo::AccessType::Read,
{AccessRange.value()},
/*IsClobber=*/false};
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No else after return

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

return {ArgumentAccessInfo::AccessType::Read, {}, /*IsClobber=*/true};
}
}
} else if (auto *CB = dyn_cast<CallBase>(I)) {
if (CB->isArgOperand(IU.U)) {
unsigned ArgNo = CB->getArgOperandNo(IU.U);
bool IsInitialize = CB->paramHasAttr(ArgNo, Attribute::Initializes);
// Argument is only not clobbered when parameter is writeonly/readnone
// and nocapture.
bool IsClobber = !(CB->onlyWritesMemory(ArgNo) &&
CB->paramHasAttr(ArgNo, Attribute::NoCapture));
ConstantRangeList AccessRanges;
if (IsInitialize && IU.Offset.has_value()) {
Attribute Attr = CB->getParamAttr(ArgNo, Attribute::Initializes);
if (!Attr.isValid()) {
Attr = CB->getCalledFunction()->getParamAttribute(
ArgNo, Attribute::Initializes);
}
ConstantRangeList CBCRL = Attr.getValueAsConstantRangeList();
for (ConstantRange &CR : CBCRL) {
AccessRanges.insert(ConstantRange(CR.getLower() + IU.Offset.value(),
CR.getUpper() + IU.Offset.value()));
}
return {ArgumentAccessInfo::AccessType::Write, AccessRanges, IsClobber};
}
}
}
// Unrecognized instructions are considered clobbers.
return {ArgumentAccessInfo::AccessType::Unknown, {}, /*IsClobber=*/true};
}

std::pair<bool, bool> CollectArgumentUsesPerBlock(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some doc for the function? What does the return value represent? UsesPerBlock updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this return UsesPerBlock along with the two bools, rather than using an out parameter? and maybe change the return type to a struct with nice names rather than std::pair/tuple

Argument &A, Function &F,
DenseMap<const BasicBlock *, UsesPerBlockInfo> &UsesPerBlock) {
auto &DL = F.getParent()->getDataLayout();
auto PointerSize =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto PointerSize =
unsigned PointerSize =

DL.getIndexSizeInBits(A.getType()->getPointerAddressSpace());

bool HasAnyWrite = false;
bool HasWriteOutsideEntryBB = false;

BasicBlock &EntryBB = F.getEntryBlock();
SmallVector<ArgumentUse, 4> Worklist;
for (Use &U : A.uses())
Worklist.push_back({&U, 0});

auto UpdateUseInfo = [&UsesPerBlock](Instruction *I,
jvoung marked this conversation as resolved.
Show resolved Hide resolved
ArgumentAccessInfo Info) {
auto *BB = I->getParent();
auto &BBInfo = UsesPerBlock.getOrInsertDefault(BB);
jvoung marked this conversation as resolved.
Show resolved Hide resolved
bool AlreadyVisitedInst = BBInfo.Insts.contains(I);
auto &IInfo = BBInfo.Insts[I];

// Instructions that have more than one use of the argument are considered
// as clobbers.
if (AlreadyVisitedInst) {
IInfo = {ArgumentAccessInfo::AccessType::Unknown, {}, true};
BBInfo.HasClobber = true;
return false;
}

IInfo = Info;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
IInfo = Info;
IInfo = std::move(Info);

BBInfo.HasClobber |= IInfo.IsClobber;
BBInfo.HasWrites |=
(IInfo.ArgAccessType == ArgumentAccessInfo::AccessType::Write &&
!IInfo.AccessRanges.empty());
return !IInfo.AccessRanges.empty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the return value mean "HasWrite" or is it more subtle than that? If it means "HasWrite", should it match the "BBInfo.HasWrites " above in that it also checks ArgAccessType == Write ?

Copy link
Contributor Author

@haopliu haopliu Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Updated.

};

// No need for a visited set because we don't look through phis, so there are
// no cycles.
while (!Worklist.empty()) {
ArgumentUse IU = Worklist.pop_back_val();
User *U = IU.U->getUser();
// Add GEP uses to worklist.
// If the GEP is not a constant GEP, set IsInitialize to false.
jvoung marked this conversation as resolved.
Show resolved Hide resolved
if (auto *GEP = dyn_cast<GEPOperator>(U)) {
APInt Offset(PointerSize, 0, /*isSigned=*/true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
APInt Offset(PointerSize, 0, /*isSigned=*/true);
APInt Offset(PointerSize, 0);

The flag is not meaningful for a zero value.

bool IsConstGEP = GEP->accumulateConstantOffset(DL, Offset);
std::optional<int64_t> NewOffset = std::nullopt;
if (IsConstGEP && IU.Offset.has_value()) {
NewOffset = *IU.Offset + Offset.getSExtValue();
}
for (Use &U : GEP->uses())
Worklist.push_back({&U, NewOffset});
continue;
}

auto *I = cast<Instruction>(U);
bool HasWrite = UpdateUseInfo(I, GetArgmentAccessInfo(I, IU, DL));

HasAnyWrite |= HasWrite;

if (HasWrite && I->getParent() != &EntryBB) {
HasWriteOutsideEntryBB = true;
}
}
return {HasAnyWrite, HasWriteOutsideEntryBB};
}

} // end anonymous namespace

namespace llvm {
Expand Down Expand Up @@ -866,9 +1067,132 @@ static bool addAccessAttr(Argument *A, Attribute::AttrKind R) {
return true;
}

static bool inferInitializes(Argument &A, Function &F) {
DenseMap<const BasicBlock *, UsesPerBlockInfo> UsesPerBlock;
auto [HasAnyWrite, HasWriteOutsideEntryBB] =
CollectArgumentUsesPerBlock(A, F, UsesPerBlock);
// No write anywhere in the function, bail.
if (!HasAnyWrite)
return false;

BasicBlock &EntryBB = F.getEntryBlock();
DenseMap<const BasicBlock *, ConstantRangeList> Initialized;
auto VisitBlock = [&](const BasicBlock *BB) -> ConstantRangeList {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment on what VisitBlock does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and found a corner case :-)

"If this block has uses and none are writes, the argument is not initialized in this block."
Removed this early return. See the test.

auto UPB = UsesPerBlock.find(BB);

// If this block has uses and none are writes, the argument is not
// initialized in this block.
if (UPB != UsesPerBlock.end() && !UPB->second.HasWrites)
return ConstantRangeList();

ConstantRangeList CRL;

// Start with intersection of successors.
// If this block has any clobbering use, we're going to clear out the
// ranges at some point in this block anyway, so don't bother looking at
// successors.
if (UPB == UsesPerBlock.end() || !UPB->second.HasClobber) {
bool HasAddedSuccessor = false;
for (auto *Succ : successors(BB)) {
if (auto SuccI = Initialized.find(Succ); SuccI != Initialized.end()) {
if (HasAddedSuccessor) {
CRL = CRL.intersectWith(SuccI->second);
} else {
CRL = SuccI->second;
HasAddedSuccessor = true;
}
} else {
CRL = ConstantRangeList();
break;
}
}
}

if (UPB != UsesPerBlock.end()) {
// Sort uses in this block by instruction order.
SmallVector<std::pair<Instruction *, ArgumentAccessInfo>, 2> Insts;
append_range(Insts, UPB->second.Insts);
sort(Insts, [](std::pair<Instruction *, ArgumentAccessInfo> &LHS,
std::pair<Instruction *, ArgumentAccessInfo> &RHS) {
return LHS.first->comesBefore(RHS.first);
});

// From the end of the block to the beginning of the block, set
// initializes ranges.
for (auto [_, Info] : reverse(Insts)) {
jvoung marked this conversation as resolved.
Show resolved Hide resolved
if (Info.IsClobber) {
CRL = ConstantRangeList();
}
if (!Info.AccessRanges.empty()) {
if (Info.ArgAccessType == ArgumentAccessInfo::AccessType::Write) {
CRL = CRL.unionWith(Info.AccessRanges);
} else {
assert(Info.ArgAccessType == ArgumentAccessInfo::AccessType::Read);
for (const auto &ReadRange : Info.AccessRanges) {
CRL.subtract(ReadRange);
}
}
}
}
}
return CRL;
};

ConstantRangeList EntryCRL;
// If all write instructions are in the EntryBB, or if the EntryBB has
// a clobbering use, we only need to look at EntryBB.
bool OnlyScanEntryBlock = !HasWriteOutsideEntryBB;
if (!OnlyScanEntryBlock) {
if (auto EntryUPB = UsesPerBlock.find(&EntryBB);
EntryUPB != UsesPerBlock.end()) {
OnlyScanEntryBlock = EntryUPB->second.HasClobber;
}
}
if (OnlyScanEntryBlock) {
EntryCRL = VisitBlock(&EntryBB);
if (EntryCRL.empty()) {
return false;
}
} else {
// Visit successors before predecessors with a post-order walk of the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you expand the comment to include the reasoning behind this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

// blocks.
for (const BasicBlock *BB : post_order(&F)) {
ConstantRangeList CRL = VisitBlock(BB);
if (!CRL.empty()) {
Initialized[BB] = CRL;
}
}

auto EntryCRLI = Initialized.find(&EntryBB);
if (EntryCRLI == Initialized.end()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks for reminding! Done.

return false;
}

EntryCRL = EntryCRLI->second;
}

assert(!EntryCRL.empty() &&
"should have bailed already if EntryCRL is empty");

if (A.hasAttribute(Attribute::Initializes)) {
ConstantRangeList PreviousCRL =
A.getAttribute(Attribute::Initializes).getValueAsConstantRangeList();
if (PreviousCRL == EntryCRL) {
return false;
}
EntryCRL = EntryCRL.unionWith(PreviousCRL);
}

A.addAttr(Attribute::get(A.getContext(), Attribute::Initializes,
EntryCRL.rangesRef()));

return true;
}

/// Deduce nocapture attributes for the SCC.
static void addArgumentAttrs(const SCCNodeSet &SCCNodes,
SmallSet<Function *, 8> &Changed) {
SmallSet<Function *, 8> &Changed,
bool SkipInitializes) {
ArgumentGraph AG;

// Check each function in turn, determining which pointer arguments are not
Expand Down Expand Up @@ -936,6 +1260,10 @@ static void addArgumentAttrs(const SCCNodeSet &SCCNodes,
if (addAccessAttr(&A, R))
Changed.insert(F);
}
if (!SkipInitializes && !A.onlyReadsMemory()) {
if (inferInitializes(A, *F))
Changed.insert(F);
}
}
}

Expand Down Expand Up @@ -1844,13 +2172,13 @@ deriveAttrsInPostOrder(ArrayRef<Function *> Functions, AARGetterT &&AARGetter,

SmallSet<Function *, 8> Changed;
if (ArgAttrsOnly) {
addArgumentAttrs(Nodes.SCCNodes, Changed);
addArgumentAttrs(Nodes.SCCNodes, Changed, /*SkipInitializes=*/true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment as to why SkipInitializes in the ArgAttrsOnly case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :-D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would simplify the comment a bit, this is too much detail. The important thing is that ArgAttrsOnly means to only infer attributes that may aid optimizations on the current function. initializes does not fall into that category.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

return Changed;
}

addArgumentReturnedAttrs(Nodes.SCCNodes, Changed);
addMemoryAttrs(Nodes.SCCNodes, AARGetter, Changed);
addArgumentAttrs(Nodes.SCCNodes, Changed);
addArgumentAttrs(Nodes.SCCNodes, Changed, /*SkipInitializes=*/false);
inferConvergent(Nodes.SCCNodes, Changed);
addNoReturnAttrs(Nodes.SCCNodes, Changed);
addWillReturn(Nodes.SCCNodes, Changed);
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ define void @test0_yes(ptr %p) nounwind {
ret void
}

; CHECK: define void @test0_no(ptr nocapture writeonly %p) #1 {
; CHECK: define void @test0_no(ptr nocapture writeonly initializes((0, 4)) %p) #1 {
define void @test0_no(ptr %p) nounwind {
store i32 0, ptr %p, !tbaa !2
ret void
Expand Down
Loading
Loading