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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 2 inline comments as done.
mtrofin 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77877