Skip to content

Commit

Permalink
[Attributor][FIX] Pipe UsedAssumedInformation through more interfaces
Browse files Browse the repository at this point in the history
`UsedAssumedInformation` is a return argument utilized to determine what
information is known. Most APIs used it already but
`genericValueTraversal` did not. This adds it to `genericValueTraversal`
and replaces `AllCallSitesKnown` of `checkForAllCallSites` with the
commonly used `UsedAssumedInformation`.

This was supposed to be a NFC commit, then the test change appeared.
Turns out, we had one user of `AllCallSitesKnown` (AANoReturn) and the
way we set `AllCallSitesKnown` was wrong as we ignored the fact some
call sites were optimistically assumed dead. Included a dedicated test
for this as well now.

Fixes #53884
  • Loading branch information
jdoerfert authored and tstellar committed Mar 1, 2022
1 parent 4327d39 commit f58ab32
Show file tree
Hide file tree
Showing 5 changed files with 143 additions and 84 deletions.
14 changes: 8 additions & 6 deletions llvm/include/llvm/Transforms/IPO/Attributor.h
Expand Up @@ -192,6 +192,7 @@ bool getAssumedUnderlyingObjects(Attributor &A, const Value &Ptr,
SmallVectorImpl<Value *> &Objects,
const AbstractAttribute &QueryingAA,
const Instruction *CtxI,
bool &UsedAssumedInformation,
bool Intraprocedural = false);

/// Collect all potential values of the one stored by \p SI into
Expand Down Expand Up @@ -1824,23 +1825,24 @@ struct Attributor {
/// This method will evaluate \p Pred on call sites and return
/// true if \p Pred holds in every call sites. However, this is only possible
/// all call sites are known, hence the function has internal linkage.
/// If true is returned, \p AllCallSitesKnown is set if all possible call
/// sites of the function have been visited.
/// If true is returned, \p UsedAssumedInformation is set if assumed
/// information was used to skip or simplify potential call sites.
bool checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
const AbstractAttribute &QueryingAA,
bool RequireAllCallSites, bool &AllCallSitesKnown);
bool RequireAllCallSites,
bool &UsedAssumedInformation);

/// Check \p Pred on all call sites of \p Fn.
///
/// This method will evaluate \p Pred on call sites and return
/// true if \p Pred holds in every call sites. However, this is only possible
/// all call sites are known, hence the function has internal linkage.
/// If true is returned, \p AllCallSitesKnown is set if all possible call
/// sites of the function have been visited.
/// If true is returned, \p UsedAssumedInformation is set if assumed
/// information was used to skip or simplify potential call sites.
bool checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
const Function &Fn, bool RequireAllCallSites,
const AbstractAttribute *QueryingAA,
bool &AllCallSitesKnown);
bool &UsedAssumedInformation);

/// Check \p Pred on all values potentially returned by \p F.
///
Expand Down
32 changes: 13 additions & 19 deletions llvm/lib/Transforms/IPO/Attributor.cpp
Expand Up @@ -320,7 +320,8 @@ bool AA::getPotentialCopiesOfStoredValue(

Value &Ptr = *SI.getPointerOperand();
SmallVector<Value *, 8> Objects;
if (!AA::getAssumedUnderlyingObjects(A, Ptr, Objects, QueryingAA, &SI)) {
if (!AA::getAssumedUnderlyingObjects(A, Ptr, Objects, QueryingAA, &SI,
UsedAssumedInformation)) {
LLVM_DEBUG(
dbgs() << "Underlying objects stored into could not be determined\n";);
return false;
Expand Down Expand Up @@ -514,10 +515,10 @@ isPotentiallyReachable(Attributor &A, const Instruction &FromI,
return true;
};

bool AllCallSitesKnown;
bool UsedAssumedInformation = false;
Result = !A.checkForAllCallSites(CheckCallSite, *FromFn,
/* RequireAllCallSites */ true,
&QueryingAA, AllCallSitesKnown);
&QueryingAA, UsedAssumedInformation);
if (Result) {
LLVM_DEBUG(dbgs() << "[AA] stepping back to call sites from " << *CurFromI
<< " in @" << FromFn->getName()
Expand Down Expand Up @@ -1277,7 +1278,7 @@ bool Attributor::checkForAllUses(
bool Attributor::checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
const AbstractAttribute &QueryingAA,
bool RequireAllCallSites,
bool &AllCallSitesKnown) {
bool &UsedAssumedInformation) {
// We can try to determine information from
// the call sites. However, this is only possible all call sites are known,
// hence the function has internal linkage.
Expand All @@ -1286,31 +1287,26 @@ bool Attributor::checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
if (!AssociatedFunction) {
LLVM_DEBUG(dbgs() << "[Attributor] No function associated with " << IRP
<< "\n");
AllCallSitesKnown = false;
return false;
}

return checkForAllCallSites(Pred, *AssociatedFunction, RequireAllCallSites,
&QueryingAA, AllCallSitesKnown);
&QueryingAA, UsedAssumedInformation);
}

bool Attributor::checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
const Function &Fn,
bool RequireAllCallSites,
const AbstractAttribute *QueryingAA,
bool &AllCallSitesKnown) {
bool &UsedAssumedInformation) {
if (RequireAllCallSites && !Fn.hasLocalLinkage()) {
LLVM_DEBUG(
dbgs()
<< "[Attributor] Function " << Fn.getName()
<< " has no internal linkage, hence not all call sites are known\n");
AllCallSitesKnown = false;
return false;
}

// If we do not require all call sites we might not see all.
AllCallSitesKnown = RequireAllCallSites;

SmallVector<const Use *, 8> Uses(make_pointer_range(Fn.uses()));
for (unsigned u = 0; u < Uses.size(); ++u) {
const Use &U = *Uses[u];
Expand All @@ -1322,7 +1318,6 @@ bool Attributor::checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
dbgs() << "[Attributor] Check use: " << *U << " in " << *U.getUser()
<< "\n";
});
bool UsedAssumedInformation = false;
if (isAssumedDead(U, QueryingAA, nullptr, UsedAssumedInformation,
/* CheckBBLivenessOnly */ true)) {
LLVM_DEBUG(dbgs() << "[Attributor] Dead use, skip!\n");
Expand Down Expand Up @@ -1795,15 +1790,15 @@ void Attributor::identifyDeadInternalFunctions() {
if (!F)
continue;

bool AllCallSitesKnown;
bool UsedAssumedInformation = false;
if (checkForAllCallSites(
[&](AbstractCallSite ACS) {
Function *Callee = ACS.getInstruction()->getFunction();
return ToBeDeletedFunctions.count(Callee) ||
(Functions.count(Callee) && Callee->hasLocalLinkage() &&
!LiveInternalFns.count(Callee));
},
*F, true, nullptr, AllCallSitesKnown)) {
*F, true, nullptr, UsedAssumedInformation)) {
continue;
}

Expand Down Expand Up @@ -2290,9 +2285,9 @@ bool Attributor::isValidFunctionSignatureRewrite(
}

// Avoid callbacks for now.
bool AllCallSitesKnown;
bool UsedAssumedInformation = false;
if (!checkForAllCallSites(CallSiteCanBeChanged, *Fn, true, nullptr,
AllCallSitesKnown)) {
UsedAssumedInformation)) {
LLVM_DEBUG(dbgs() << "[Attributor] Cannot rewrite all call sites\n");
return false;
}
Expand All @@ -2305,7 +2300,6 @@ bool Attributor::isValidFunctionSignatureRewrite(

// Forbid must-tail calls for now.
// TODO:
bool UsedAssumedInformation = false;
auto &OpcodeInstMap = InfoCache.getOpcodeInstMapForFunction(*Fn);
if (!checkForAllInstructionsImpl(nullptr, OpcodeInstMap, InstPred, nullptr,
nullptr, {Instruction::Call},
Expand Down Expand Up @@ -2514,9 +2508,9 @@ ChangeStatus Attributor::rewriteFunctionSignatures(
};

// Use the CallSiteReplacementCreator to create replacement call sites.
bool AllCallSitesKnown;
bool UsedAssumedInformation = false;
bool Success = checkForAllCallSites(CallSiteReplacementCreator, *OldFn,
true, nullptr, AllCallSitesKnown);
true, nullptr, UsedAssumedInformation);
(void)Success;
assert(Success && "Assumed call site replacement to succeed!");

Expand Down

0 comments on commit f58ab32

Please sign in to comment.