[PATCH] D89587: [CGSCC] Detect devirtualization in more cases

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In D89587#2398531 <https://reviews.llvm.org/D89587#2398531>, @asbirlea wrote:

> Could you clarify why a map and not a SmallVector?
>
> I was expecting the indirect calls to be added by all CGSCC passes, e.g. `lib/Transforms/IPO/Inliner.cpp:750`. Adding the calls in `updateCG...` instead, introduces a single point of failure so less chances of having passes forget to add them; the one thing I'm not clear is if the `updateCG...` call may be added too late in a CGSCC pass; for the inliner, it seems fine (thank you for adding the tests) but I'm not clear if this will hold for other passes. I don't have a use case now, just making a note to verify this for coroutines.
> The check that all accesses are still indirect on the return looks good.

It is definitely possible to have a call to `updateCG...` miss a devirtualization in a CGSCC pass if an indirect call is introduced and then promoted before the call to `updateCG...`, but the inliner looks ok. I think other CGSCC passes don't really devirtualize much. As long as we call `updateCG...` after passes that can devirtualize (inliner, function passes like instcombine, etc) we should be good. And we can always revisit if we find missed cases.



================
Comment at: llvm/include/llvm/Analysis/CGSCCPassManager.h:321
+  /// devirtualization.
+  SmallMapVector<Value *, WeakTrackingVH, 16> IndirectVHs;
 };
----------------
rnk wrote:
> Using a Value pointer as a key seems like it could suffer from the ABA problem. Suppose a CallInst is deallocated and another is reallocated at the same address. The map will contain a false entry for the new call.
> 
> It seems like the lookup functionality (find/insert) is only needed during the update scan. You could build a map from Value* to handle index before that loop, and then do the instruction scan.
If a CallInst is deallocated, its corresponding WeakTrackingVH will become null, and that should be ok. We just want to make sure that we don't insert duplicate entries into IndirectVHs. If we never end up with a new CallInst at the same address, then we'll ignore the nulled WeakTrackingVH when scanning for promoted calls. If there is a new CallInst at the same address, it will overwrite the old one due to the `!Entry->second` check in updateCGAndAnalysisManager.

We could remove all null WeakTrackingVHs at the beginning of updateCGAndAnalysisManager to alleviate any ABA and memory (we never remove entries) concerns, although I was trying to avoid looking through all entries in WeakTrackingVHs on every call to updateCGAndAnalysisManager.

I tried changing IndirectVHs into a SmallVector<WeakTrackingVH>, but then we end up adding extra code (and runtime) to create a map on every call to updateCGAndAnalysisManager, and we might as well just have that map as IndirectVHs itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89587