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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77877