[PATCH] D105733: [OpaquePtr] Require matching signature in getCalledFunction()

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D105733#2871922 <https://reviews.llvm.org/D105733#2871922>, @dexonsmith wrote:

> In D105733#2868361 <https://reviews.llvm.org/D105733#2868361>, @nikic wrote:
>
>> I think that would be best served by a different function. This patch preserves the current behavior of `getCalledFunction()` with opaque pointers (no "bitcasts" allowed) -- we could add an additional function like `getCalledFunctionStripCasts()` that ignores bitcasts (or for opaque pointers, allows signature mismatch) and use that where profitable. Worth noting that we already have isIndirectCall(), which is not the same as getCalledFunction()!=nullptr, that one does consider bitcasts not to be indirect.
>
> I had a more through look at callers in `llvm/`. I think most callers would want to be updated to `getCalledFunctionStripCasts()`. There are many callers (especially in Analysis and Transforms) that use this as a shortcut to detect if the call is direct (existence of `CallBase::isIndirectCall()` notwithstanding). The only caller I could find that clearly knew that `nullptr` could mean something other than "not a direct call" was `computeFunctionSummary` in llvm/lib/Analysis/ModuleSummaryAnalysis.cpp (which then does does the strip itself).
>
> There are a few callers that would be broken by adjusting the behaviour, mostly due to assuming that parameter attributes are valid. Maybe those callers could be moved to a new function, and the current function's behaviour changed to match most callers' expectations? (Even these probably want to strip pointer casts as long as the called function is compatible...)
>
> I feel like this API is an opportunity to reduce the hard-to-reason-about optimization churn caused by flipping the ForceOpaquePtrs flag. With the current patch / status quo, `--force-opaque-pointers` will cause a bunch of Analysis/Transforms/etc. to learn about a bunch of direct calls they previously thought were indirect. Alternatively, stripping pointer casts in `CallBase::getCalledFunction` first would separate the direct-call-related churn from the opaque pointers feature.
>
> What do others think?

I'd generally agree with your thoughts there - especially if it means we can separate the direct-call-related churn from the opaque pointers feature.


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

https://reviews.llvm.org/D105733