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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:616-619
+      int InlineHistoryID;
+      CallBase *Instr;
+      std::tie(Instr, InlineHistoryID) = CallSites[CSi];
+      CallBase &CS = *Instr;
----------------
mtrofin wrote:
> dblaikie wrote:
> > 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.
> 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.
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.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1041
+      CallBase *CS = P.first;
+      const int InlineHistoryID = P.second;
       Function &Callee = *CS->getCalledFunction();
----------------
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 :) )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77877/new/

https://reviews.llvm.org/D77877