[llvm][NFC] Inliner.cpp: ensure InlineHistory ID is always initialized;

[mtrofin]

Summary

The inline history is associated with a call site. There are two locations
we fetch inline history. In one, we fetch it together with the call
site. In the other, we initialize it under certain conditions, use it
later under same conditions (different if check), and otherwise is
uninitialized. Although currently there is no uninitialized use, the
code is more challenging to maintain correctly, than if the value were
always initialized.

Changed to the upfront initialization pattern already present in this
file.

[davidxl]

Looks like you really don't like uninitialized locals -- but this change looks fine (wont block msan etc) :)

[dblaikie]

This would still make MSan unable to diagnose the bugs it could diagnose in the original code (If a new use of InlineHistoryID was introduced outside the IsTriviallDead condition - in the current code that could be caught by MSan or the Static Analyzer - now it'll be indistinguishable from an intentional use)

I don't think these changes are consistent with an "upfront initialization pattern" - generally variables are declared as they're needed ("Caller" is declared a little early, but otherwise the variables here aren't declared at the top of the scope of this loop unless they're needed there), and I think it's particularly different declaring/initializing essentially const helper local variables ("here's a convenient name") versus mutating state like InlineHistoryID.

If the spooky-action-at-a-distance is sufficiently risky (that the two conditions might get out of sync) I'd be inclined to use Optional here, so it's checked on access and fails on a +Asserts build if a new un-paired use is introduced, and generally keep variables scopes as short as reasonable (especially for mutating state, less so for "here's a convenient name" sort of names, if they make more sense grouped together somewhere, etc.

I'm not sure why "Instr" is even a variable... ah, probably because of the old CallSite version of this code. With CallBase in use, should probably just pass "&CB" as the parameter to isInstructionTriviallyDead...

Looking more at the code, yeah, I guess I don't mind the change so much (you're right, both these things come straight out of the vector/current element anyway - and, yes, initializing those elements at the start of the loop is quite common/reasonable/readable (std::tie/pointer/reference dance notwithstanding)) - though the motivation to initialize things that the code, when correct, doesn't need to initialize still makes me uncomfortable & it's a habit/routine I'd like to discourage so it doesn't become common & defeat bug finding tools we use. I've said my peace on that & I'll leave this up to you.

llvm/lib/Transforms/IPO/Inliner.cpp

616–619

This seems a bit convoluted. Given CallSites is a SmallVector, so [] lookup is cheap, I'd either write this with two array lookups:

CallBase &CB = *CallSites[CSi].first;
int InlineHistoryID = CallSites[CSi].second;

Or as you've done, but without "CS" (& rename Instr to CB) - having both the pointer and reference version of the variable seems like overkill/confusing. (I think the "Instr" variable existed in the original code due to CallSite usage, and is a bit vestigial now with CallBase being used - if you end up with a CallBase& in this code, then "isInstructionTriviallDead" should be called with "&CB" rather than needing a separate named variable that's a pointer version of the CallBase&)

Also the name "CS" is a bit erroneous now that this is CallBase, not CallSite.

[davidxl]

This change essentially makes a lazy initialization to become eagerly initialized. It may be better to keep the 'laziness' here, so why not just push the declaration of InlineHistoryID into the if(IsTrivialllyDead) {} block?

[davidxl]

Re: "This would still make MSan unable to diagnose the bugs it could diagnose in the original code (If a new use of InlineHistoryID was introduced outside the IsTriviallDead condition - in the current code that could be caught by MSan or the Static Analyzer - now it'll be indistinguishable from an intentional use)"

I think the ultimate goal is to have logically correct code, and making *San happy is the secondary goal which choosing implementations which can not be proved to be correct statically. In the original change, the two choices are 1) one with uninitialized variable use, and 2) one with possible wrong init value. In this case, we should prefer choice 1 to help facilitate finding the bug at runtime or some other choice that can expose the bug in running the Assert build.

For the latest change, we can statically prove it is always correct -- as InlineHistoryID is attribute associated with the callsite. For the case you describe where there is new use in a different path, the right fix will be to eagerly initialize the ID too.

[mtrofin]

In D77877#1974573, @davidxl wrote:

This change essentially makes a lazy initialization to become eagerly initialized. It may be better to keep the 'laziness' here, so why not just push the declaration of InlineHistoryID into the if(IsTrivialllyDead) {} block?

I believe the value of the CL is improved readability / maintainability: one doesn't need to trace through the code to understand where the value of InlineHistoryID comes from, and what value it is. In fact, looking at it more, we could even mark it const.

Not sure what the value of lazy initing would be here. If the initialization doesn't end up being optimized equivalently, I doubt there's a meaningful performance impact.

[mtrofin]

llvm/lib/Transforms/IPO/Inliner.cpp

616–619

There is an existing place (line 1043) doing the std::tie thing. I thought that was a preferred way, but if that's not the case, I could see doing in both places either what you're describing, or:

auto &P = CallSites[CSi];
CallBase &CG = *P.first;
int InlineHistoryID = P.second;

Happy to change it, and yank out vestigials.

CS: sure, but that's a larger change throughout, if we wanted to be consistent. On the other hand, the meaning of the variable is that it's a "call site" (the concept, not the type), so calling it 'CS' is, I'd say, reasonable.

[davidxl]

Lazy declaration is commonly used in C++ code. In general it has the advantage of limiting the life ranges of locals and reduce register pressure. Here we have a lazy declaration and also a lazy initialization but scoped, making the declaration also scoped can avoid introducing uninitialized variable use accidentally in the future.

On the other hand, I don't have strong opinion on this so it is up to you.

[mtrofin]

no need for std::tie

[dblaikie]

Dealer's choice here, especially given the fact that it's just accessing/aliasing a member of the object being iterated over anyway - mostly wanted to avoid tool-based warnings maybe encouraging a style that would be systematically applied & go in not the best direction, imho.

(the fact that the value is then mutated makes it trickier for me - it's no longer the element you're iterating over, it's got state that was only established in one codepath & only usable on a matching codepath, etc... but yep, if the code had been written this new way from the start I doubt I'd have batted an eyelid/made any point that it should be written the old way just to make MSan able to find a certain set of possible bugs)

[mtrofin]

const -ed a few values

[dblaikie]

llvm/lib/Transforms/IPO/Inliner.cpp

616–619

Yeah, I was going to go look at how the other std::tie evolved - and I think it's more legible without doing the pointer-to-reference dance too. (I probably wouldn't've objected to it written that way in the initial coding - just reviewing changes like this makes it a bit more visible, etc)

I think tie+letting it be a pointer, your solution, two accesses to Callsites as I suggested, all seem pretty fine.

1041

FWIW LLVM doesn't usually use top-level const like that - because it's so rare it can be mistaken for an accidental missing const /reference/

(but the change above does clarify a thing I was clearly misunderstanding in my head - I'd kept thinking for some reason that inlineHistoryIncludes might be able to modify InlineHistoryID somehow - so that InlineHistoryID's state was important in how it came out of that first " if (!IsTriviallyDead) {" - which I had completely wrong & removes the last slight hiccup I had around that variable - but I don't think marking it "const" is necessary to clarify that, I get that I was wrong now :) )

[mtrofin]

llvm/lib/Transforms/IPO/Inliner.cpp

1041

Ah, ok - I'll yank out the const on the next pass here.

llvm/lib/Transforms/IPO/Inliner.cpp

inlineCallsImpl(CallGraphSCC &SCC, CallGraph &CG,
   bool Changed = false;
   bool LocalChange;
   do {
     LocalChange = false;
     // Iterate over the outer loop because inlining functions can cause indirect
     // calls to become direct calls.
     // CallSites may be modified inside so ranged for loop can not be used.
     for (unsigned CSi = 0; CSi != CallSites.size(); ++CSi) {
-      CallBase &CS = *CallSites[CSi].first;
+      auto &P = CallSites[CSi];
+      CallBase &CS = *P.first;
+      const int InlineHistoryID = P.second;
 
       Function *Caller = CS.getCaller();
       Function *Callee = CS.getCalledFunction();
 
       // We can only inline direct calls to non-declarations.
       if (!Callee || Callee->isDeclaration())
         continue;
 
-      Instruction *Instr = &CS;
+      bool IsTriviallyDead = isInstructionTriviallyDead(&CS, &GetTLI(*Caller));
 
-      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)) {
           setInlineRemark(CS, "recursive");
           continue;
         }
       }
 
       // FIXME for new PM: because of the old PM we currently generate ORE and
for (unsigned CSi = 0; CSi != CallSites.size(); ++CSi) {
         continue;
       }
 
       // If this call site is dead and it is to a readonly function, we should
       // just delete the call instead of trying to inline it, regardless of
       // size.  This happens because IPSCCP propagates the result out of the
       // call and then we're left with the dead call.
       if (IsTriviallyDead) {
-        LLVM_DEBUG(dbgs() << "    -> Deleting dead call: " << *Instr << "\n");
+        LLVM_DEBUG(dbgs() << "    -> Deleting dead call: " << CS << "\n");
         // Update the call graph by deleting the edge from Callee to Caller.
         setInlineRemark(CS, "trivially dead");
         CG[Caller]->removeCallEdgeFor(CS);
-        Instr->eraseFromParent();
+        CS.eraseFromParent();
         ++NumCallsDeleted;
       } 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;
auto GetInlineCost = [&](CallBase &CS) {
                            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) {
-      int InlineHistoryID;
-      CallBase *CS = nullptr;
-      std::tie(CS, InlineHistoryID) = Calls[I];
+      auto &P = Calls[I];
+      CallBase *CS = P.first;
+      const int InlineHistoryID = P.second;
       Function &Callee = *CS->getCalledFunction();
 
       if (InlineHistoryID != -1 &&
           inlineHistoryIncludes(&Callee, InlineHistoryID, InlineHistory)) {
         setInlineRemark(*CS, "recursive");
         continue;
       }