[llvm][NFC] Style fixes in Inliner.cpp

[mtrofin]

Summary

Function names: camel case, lower case first letter.
Variable names: start with upper letter. For iterators that were 'i',
renamed with a descriptive name, as 'I' is 'Instruction&'.

Lambda captures simplification.

Opportunistic boolean return simplification.

[dblaikie]

Looks good - few optional bits & pieces (the initialization's probably something I feel pretty strongly about - but naming you can take/leave as you see fit).

In general fixing names to match the LLVM naming conventions - feel free to commit that sort of thing without review, usually independent changes independently - but if you're fixing a class of bugs (like a whole bunch of functions in one class, etc) you can do it in pretty big chunks (whole class/file at a time, potentially - but don't feel like you have to fix everything or nothing).

llvm/lib/Transforms/IPO/Inliner.cpp

598–599

"I" would suffice here (in general I think of shorter variables for shorter scopes) - the LLVM style guide does provide examples with single uppercase (I thought there used to be some special case for lower case index variables - maybe that was removed with the advent of range-based for loops and such, or I'm misremembering): https://llvm.org/docs/CodingStandards.html#turn-predicate-loops-into-predicate-functions

630

Generally I'm in favor of not adding unneeded (in the sense that if the program is working as intended it won't read uninitialized values) - that way tools like MSan and the like can find bugs in this code if the program isn't working as intended.

Looks like that's the case here - InlineHistoryID is only used under the same condition (IsTriviallyDead) it's initialized. (also Clang has warnings for this too, in addition to the runtime checks)

986

Again, probably using just I is fine here. On the other hand it is a rather long loop that does some non-trivial things with this loop index, so if you prefer the longer name - I quite understand.

[mtrofin]

llvm/lib/Transforms/IPO/Inliner.cpp

598–599

There's a name collision for 'I'.

630

My worry is maintainability - initializing is local, while making sure that code evolution doesn't lead to uninitialized value propagating isn't.

986

Also name collision, unfortunately - I also didn't like I had to cook up these long names :)

[davidxl]

llvm/lib/Transforms/IPO/Inliner.cpp

630

This is a good point. It is ok to initialize it to illegal value that can be caught by assertions or can consistently crash the program. Here is not the case: -1 is actually a valid value to indicate a state, so it is not a good idea to initialize it to it.

[dblaikie]

llvm/lib/Transforms/IPO/Inliner.cpp

598–599

Hmm - I don't see it. There's an I at 560, but that scope ends at 590 - if I is good enough for that scope, it's good enough for this one?

Also I think it's not uncommon to use J, K etc for single name index-y variables after I, but more common if the loops are tightly coupled together - would be a bit weird to use them when they're further apart.

630

Yeah, mixed opinions.

Really pedantically you could use Optional<int> which will ensure it's checked in +Asserts build, or I guess some other invalid value (like -2), though not sure how much that'd help - it'd suppress msan and make such bugs much more elusive - maybe an infinite loop later on if inlineHistoryIncludes was called with -2? Oh, maybe buffer underrun which asan would catch.

986

I see an I from 947 to 963, but not one that seems to overlap with this line/scope.

[mtrofin]

iterators

[mtrofin]

llvm/lib/Transforms/IPO/Inliner.cpp

598–599

You are correct. I am using CLion to do the refactorings, and its naming conflict seems to be overly conservative. There's no conflict, indeed.

630

I'm removing this change from here, both because there are a few more spots I'm seeing this, and because it looks like it needs a bit more thought than appropriate for the scope of this change.

986

Yup, I'll cowardly blame the tool I used :)

[davidxl]

David is thorough in the review, so no need to wait for my lgtm, though it does lgtm :)

[dblaikie]

Reaffirming acceptance.

llvm/lib/Transforms/IPO/Inliner.cpp

598–599

Curious limitation/bug there, indeed. I wonder if there's something more powered by the Language Server Protocol and clangd that'd be more accurate in this regard...

llvm/lib/Transforms/IPO/Inliner.cpp

static void mergeInlinedArrayAllocas(
   // because their scopes are not disjoint.  We could make this smarter by
   // keeping track of the inline history for each alloca in the
   // InlinedArrayAllocas but this isn't likely to be a significant win.
   if (InlineHistory != -1) // Only do merging for top-level call sites in SCC.
     return;
 
   // Loop over all the allocas we have so far and see if they can be merged with
   // a previously inlined alloca.  If not, remember that we had it.
-  for (unsigned AllocaNo = 0, e = IFI.StaticAllocas.size(); AllocaNo != e;
+  for (unsigned AllocaNo = 0, E = IFI.StaticAllocas.size(); AllocaNo != E;
        ++AllocaNo) {
     AllocaInst *AI = IFI.StaticAllocas[AllocaNo];
 
     // Don't bother trying to merge array allocations (they will usually be
     // canonicalized to be an allocation *of* an array), or allocations whose
     // type is not itself an array (because we're afraid of pessimizing SRoA).
     ArrayType *ATy = dyn_cast<ArrayType>(AI->getAllocatedType());
     if (!ATy || AI->isArrayAllocation())

 /// If it is possible to inline the specified call site,
 /// do so and update the CallGraph for this operation.
 ///
 /// This function also does some basic book-keeping to update the IR.  The
 /// InlinedArrayAllocas map keeps track of any allocas that are already
 /// available from other functions inlined into the caller.  If we are able to
 /// inline this call site we attempt to reuse already available allocas or add
 /// any new allocas to the set if not possible.
-static InlineResult InlineCallIfPossible(
+static InlineResult inlineCallIfPossible(
     CallBase &CS, InlineFunctionInfo &IFI,
     InlinedArrayAllocasTy &InlinedArrayAllocas, int InlineHistory,
     bool InsertLifetime, function_ref<AAResults &(Function &)> &AARGetter,
     ImportedFunctionsInliningStatistics &ImportedFunctionsStats) {
   Function *Callee = CS.getCalledFunction();
   Function *Caller = CS.getCaller();
 
   AAResults &AAR = AARGetter(*Callee);
shouldBeDeferred(Function *Caller, InlineCost IC, int &TotalSecondaryCost,
   // treating them as truly abstract units etc.
   TotalSecondaryCost = 0;
   // The candidate cost to be imposed upon the current function.
   int CandidateCost = IC.getCost() - 1;
   // If the caller has local linkage and can be inlined to all its callers, we
   // can apply a huge negative bonus to TotalSecondaryCost.
   bool ApplyLastCallBonus = Caller->hasLocalLinkage() && !Caller->hasOneUse();
   // This bool tracks what happens if we DO inline C into B.
-  bool inliningPreventsSomeOuterInline = false;
+  bool InliningPreventsSomeOuterInline = false;
   for (User *U : Caller->users()) {
     // If the caller will not be removed (either because it does not have a
     // local linkage or because the LastCallToStaticBonus has been already
     // applied), then we can exit the loop early.
     if (!ApplyLastCallBonus && TotalSecondaryCost >= IC.getCost())
       return false;
     CallBase *CS2 = dyn_cast<CallBase>(U);
 
for (User *U : Caller->users()) {
     }
     if (IC2.isAlways())
       continue;
 
     // See if inlining of the original callsite would erase the cost delta of
     // this callsite. We subtract off the penalty for the call instruction,
     // which we would be deleting.
     if (IC2.getCostDelta() <= CandidateCost) {
-      inliningPreventsSomeOuterInline = true;
+      InliningPreventsSomeOuterInline = true;
       TotalSecondaryCost += IC2.getCost();
     }
   }
   // If all outer calls to Caller would get inlined, the cost for the last
   // one is set very low by getInlineCost, in anticipation that Caller will
   // be removed entirely.  We did not account for this above unless there
   // is only one caller of Caller.
   if (ApplyLastCallBonus)
     TotalSecondaryCost -= InlineConstants::LastCallToStaticBonus;
 
-  if (inliningPreventsSomeOuterInline && TotalSecondaryCost < IC.getCost())
-    return true;
-
-  return false;
+  return InliningPreventsSomeOuterInline && TotalSecondaryCost < IC.getCost();
 }
 
 static std::basic_ostream<char> &operator<<(std::basic_ostream<char> &R,
                                             const ore::NV &Arg) {
   return R << Arg.Val;
 }
 
 template <class RemarkT>
shouldInline(CallBase &CS, function_ref<InlineCost(CallBase &CS)> GetInlineCost,
 
   LLVM_DEBUG(dbgs() << "    Inlining " << inlineCostStr(IC) << ", Call: " << CS
                     << '\n');
   return IC;
 }
 
 /// Return true if the specified inline history ID
 /// indicates an inline history that includes the specified function.
-static bool InlineHistoryIncludes(
+static bool inlineHistoryIncludes(
     Function *F, int InlineHistoryID,
     const SmallVectorImpl<std::pair<Function *, int>> &InlineHistory) {
   while (InlineHistoryID != -1) {
     assert(unsigned(InlineHistoryID) < InlineHistory.size() &&
            "Invalid inline history ID");
     if (InlineHistory[InlineHistoryID].first == F)
       return true;
     InlineHistoryID = InlineHistory[InlineHistoryID].second;
   }
   return false;
 }
 
 bool LegacyInlinerBase::doInitialization(CallGraph &CG) {
   if (InlinerFunctionImportStats != InlinerFunctionImportStatsOpts::No)
     ImportedFunctionsStats.setModuleInfo(CG.getModule());
   return false; // No changes to CallGraph.
 }
 
 bool LegacyInlinerBase::runOnSCC(CallGraphSCC &SCC) {
   if (skipSCC(SCC))
     return false;
   return inlineCalls(SCC);
 }
 
-static void emit_inlined_into(OptimizationRemarkEmitter &ORE, DebugLoc &DLoc,
-                              const BasicBlock *Block, const Function &Callee,
-                              const Function &Caller, const InlineCost &IC) {
+static void emitInlinedInto(OptimizationRemarkEmitter &ORE, DebugLoc &DLoc,
   ORE.emit([&]() {
     bool AlwaysInline = IC.isAlways();
     StringRef RemarkName = AlwaysInline ? "AlwaysInline" : "Inlined";
     return OptimizationRemark(DEBUG_TYPE, RemarkName, DLoc, Block)
            << ore::NV("Callee", &Callee) << " inlined into "
            << ore::NV("Caller", &Caller) << " with " << IC;
   });
 }
 
 static void setInlineRemark(CallBase &CS, StringRef Message) {
   if (!InlineRemarkAttribute)
     return;
 
-  Attribute attr = Attribute::get(CS.getContext(), "inline-remark", Message);
-  CS.addAttribute(AttributeList::FunctionIndex, attr);
+  Attribute Attr = Attribute::get(CS.getContext(), "inline-remark", Message);
+  CS.addAttribute(AttributeList::FunctionIndex, Attr);
 }
 
 static bool
 inlineCallsImpl(CallGraphSCC &SCC, CallGraph &CG,
                 std::function<AssumptionCache &(Function &)> GetAssumptionCache,
                 ProfileSummaryInfo *PSI,
                 std::function<const TargetLibraryInfo &(Function &)> GetTLI,
                 bool InsertLifetime,
inlineCallsImpl(CallGraphSCC &SCC, CallGraph &CG,
 
   // If there are no calls in this function, exit early.
   if (CallSites.empty())
     return false;
 
   // Now that we have all of the call sites, move the ones to functions in the
   // current SCC to the end of the list.
   unsigned FirstCallInSCC = CallSites.size();
-  for (unsigned i = 0; i < FirstCallInSCC; ++i)
-    if (Function *F = CallSites[i].first->getCalledFunction())
+  for (unsigned I = 0; I < FirstCallInSCC; ++I)
+    if (Function *F = CallSites[I].first->getCalledFunction())
       if (SCCFunctions.count(F))
-        std::swap(CallSites[i--], CallSites[--FirstCallInSCC]);
+        std::swap(CallSites[I--], CallSites[--FirstCallInSCC]);
 
   InlinedArrayAllocasTy InlinedArrayAllocas;
   InlineFunctionInfo InlineInfo(&CG, &GetAssumptionCache, PSI);
 
   // Now that we have all of the call sites, loop over them and inline them if
   // it looks profitable to do so.
   bool Changed = false;
   bool LocalChange;
for (unsigned CSi = 0; CSi != CallSites.size(); ++CSi) {
       if (!Callee || Callee->isDeclaration())
         continue;
 
       Instruction *Instr = &CS;
 
       bool IsTriviallyDead =
           isInstructionTriviallyDead(Instr, &GetTLI(*Caller));
 
       int InlineHistoryID;
       if (!IsTriviallyDead) {
         // If this call site was obtained by inlining another function, verify
         // that the include path for the function did not include the callee
         // itself.  If so, we'd be recursively inlining the same function,
         // which would provide the same callsites, which would cause us to
         // infinitely inline.
         InlineHistoryID = CallSites[CSi].second;
         if (InlineHistoryID != -1 &&
-            InlineHistoryIncludes(Callee, InlineHistoryID, InlineHistory)) {
+            inlineHistoryIncludes(Callee, InlineHistoryID, InlineHistory)) {
           setInlineRemark(CS, "recursive");
           continue;
         }
       }
 
       // FIXME for new PM: because of the old PM we currently generate ORE and
       // in turn BFI on demand.  With the new PM, the ORE dependency should
       // just become a regular analysis dependency.
for (unsigned CSi = 0; CSi != CallSites.size(); ++CSi) {
       } else {
         // Get DebugLoc to report. CS will be invalid after Inliner.
         DebugLoc DLoc = CS.getDebugLoc();
         BasicBlock *Block = CS.getParent();
 
         // Attempt to inline the function.
         using namespace ore;
 
-        InlineResult IR = InlineCallIfPossible(
+        InlineResult IR = inlineCallIfPossible(
             CS, InlineInfo, InlinedArrayAllocas, InlineHistoryID,
             InsertLifetime, AARGetter, ImportedFunctionsStats);
         if (!IR.isSuccess()) {
           setInlineRemark(CS, std::string(IR.getFailureReason()) + "; " +
                                   inlineCostStr(*OIC));
           ORE.emit([&]() {
             return OptimizationRemarkMissed(DEBUG_TYPE, "NotInlined", DLoc,
                                             Block)
                    << NV("Callee", Callee) << " will not be inlined into "
                    << NV("Caller", Caller) << ": "
                    << NV("Reason", IR.getFailureReason());
           });
           continue;
         }
         ++NumInlined;
 
-        emit_inlined_into(ORE, DLoc, Block, *Callee, *Caller, *OIC);
+        emitInlinedInto(ORE, DLoc, Block, *Callee, *Caller, *OIC);
 
         // If inlining this function gave us any new call sites, throw them
         // onto our worklist to process.  They are useful inline candidates.
         if (!InlineInfo.InlinedCalls.empty()) {
           // Create a new inline history entry for this, so that we remember
           // that these new callsites came about due to inlining Callee.
           int NewHistoryID = InlineHistory.size();
           InlineHistory.push_back(std::make_pair(Callee, InlineHistoryID));
bool LegacyInlinerBase::inlineCalls(CallGraphSCC &SCC) {
   GetTLI = [&](Function &F) -> const TargetLibraryInfo & {
     return getAnalysis<TargetLibraryInfoWrapperPass>().getTLI(F);
   };
   auto GetAssumptionCache = [&](Function &F) -> AssumptionCache & {
     return ACT->getAssumptionCache(F);
   };
   return inlineCallsImpl(
       SCC, CG, GetAssumptionCache, PSI, GetTLI, InsertLifetime,
-      [this](CallBase &CS) { return getInlineCost(CS); },
-      LegacyAARGetter(*this), ImportedFunctionsStats);
+      [&](CallBase &CS) { return getInlineCost(CS); }, LegacyAARGetter(*this),
+      ImportedFunctionsStats);
 }
 
 /// Remove now-dead linkonce functions at the end of
 /// processing to avoid breaking the SCC traversal.
 bool LegacyInlinerBase::doFinalization(CallGraph &CG) {
   if (InlinerFunctionImportStats != InlinerFunctionImportStatsOpts::No)
     ImportedFunctionsStats.dump(InlinerFunctionImportStats ==
                                 InlinerFunctionImportStatsOpts::Verbose);
PreservedAnalyses InlinerPass::run(LazyCallGraph::SCC &InitialC,
   SmallSetVector<Function *, 4> InlinedCallees;
 
   // Track the dead functions to delete once finished with inlining calls. We
   // defer deleting these to make it easier to handle the call graph updates.
   SmallVector<Function *, 4> DeadFunctions;
 
   // Loop forward over all of the calls. Note that we cannot cache the size as
   // inlining can introduce new calls that need to be processed.
-  for (int i = 0; i < (int)Calls.size(); ++i) {
+  for (int I = 0; I < (int)Calls.size(); ++I) {
     // We expect the calls to typically be batched with sequences of calls that
     // have the same caller, so we first set up some shared infrastructure for
     // this caller. We also do any pruning we can at this layer on the caller
     // alone.
-    Function &F = *Calls[i].first->getCaller();
+    Function &F = *Calls[I].first->getCaller();
     LazyCallGraph::Node &N = *CG.lookup(F);
     if (CG.lookupSCC(N) != C)
       continue;
     if (F.hasOptNone()) {
-      setInlineRemark(*Calls[i].first, "optnone attribute");
+      setInlineRemark(*Calls[I].first, "optnone attribute");
       continue;
     }
 
     LLVM_DEBUG(dbgs() << "Inlining calls in: " << F.getName() << "\n");
 
     // Get a FunctionAnalysisManager via a proxy for this particular node. We
     // do this each time we visit a node as the SCC may have changed and as
     // we're going to mutate this particular function we want to make sure the
auto GetInlineCost = [&](CallBase &CS) {
       return getInlineCost(CS, Params, CalleeTTI, GetAssumptionCache, {GetBFI},
                            GetTLI, PSI, RemarksEnabled ? &ORE : nullptr);
     };
 
     // Now process as many calls as we have within this caller in the sequnece.
     // We bail out as soon as the caller has to change so we can update the
     // call graph and prepare the context of that new caller.
     bool DidInline = false;
-    for (; i < (int)Calls.size() && Calls[i].first->getCaller() == &F; ++i) {
+    for (; I < (int)Calls.size() && Calls[I].first->getCaller() == &F; ++I) {
       int InlineHistoryID;
       CallBase *CS = nullptr;
-      std::tie(CS, InlineHistoryID) = Calls[i];
+      std::tie(CS, InlineHistoryID) = Calls[I];
       Function &Callee = *CS->getCalledFunction();
 
       if (InlineHistoryID != -1 &&
-          InlineHistoryIncludes(&Callee, InlineHistoryID, InlineHistory)) {
+          inlineHistoryIncludes(&Callee, InlineHistoryID, InlineHistory)) {
         setInlineRemark(*CS, "recursive");
         continue;
       }
 
       // Check if this inlining may repeat breaking an SCC apart that has
       // already been split once before. In that case, inlining here may
       // trigger infinite inlining, much like is prevented within the inliner
       // itself by the InlineHistory above, but spread across CGSCC iterations
for (; I < (int)Calls.size() && Calls[I].first->getCaller() == &F; ++I) {
         });
         continue;
       }
       DidInline = true;
       InlinedCallees.insert(&Callee);
 
       ++NumInlined;
 
-      emit_inlined_into(ORE, DLoc, Block, Callee, F, *OIC);
+      emitInlinedInto(ORE, DLoc, Block, Callee, F, *OIC);
 
       // Add any new callsites to defined functions to the worklist.
       if (!IFI.InlinedCallSites.empty()) {
         int NewHistoryID = InlineHistory.size();
         InlineHistory.push_back({&Callee, InlineHistoryID});
 
         // FIXME(mtrofin): refactor IFI.InlinedCallSites to be CallBase-based
         for (CallSite &CS : reverse(IFI.InlinedCallSites)) {
for (; I < (int)Calls.size() && Calls[I].first->getCaller() == &F; ++I) {
       // which may reduce the number of callers of other functions to one,
       // changing inline cost thresholds.
       if (Callee.hasLocalLinkage()) {
         // To check this we also need to nuke any dead constant uses (perhaps
         // made dead by this operation on other functions).
         Callee.removeDeadConstantUsers();
         if (Callee.use_empty() && !CG.isLibFunction(Callee)) {
           Calls.erase(
-              std::remove_if(Calls.begin() + i + 1, Calls.end(),
-                             [&Callee](const std::pair<CallBase *, int> &Call) {
+              std::remove_if(Calls.begin() + I + 1, Calls.end(),
+                             [&](const std::pair<CallBase *, int> &Call) {
                                return Call.first->getCaller() == &Callee;
                              }),
               Calls.end());
           // Clear the body and queue the function itself for deletion when we
           // finish inlining and call graph updates.
           // Note that after this point, it is an error to do anything other
           // than use the callee's address or delete it.
           Callee.dropAllReferences();
           assert(find(DeadFunctions, &Callee) == DeadFunctions.end() &&
                  "Cannot put cause a function to become dead twice!");
           DeadFunctions.push_back(&Callee);
         }
       }
     }
 
     // Back the call index up by one to put us in a good position to go around
     // the outer loop.
-    --i;
+    --I;
 
     if (!DidInline)
       continue;
     Changed = true;
 
     // Add all the inlined callees' edges as ref edges to the caller. These are
     // by definition trivial edges as we always have *some* transitive ref edge
     // chain. While in some cases these edges are direct calls inside the