[PATCH] D82572: [CallGraph] Add support for callback call sites

[Sergey Dmitriev via Phabricator via llvm-commits]

sdmitriev added inline comments.


================
Comment at: llvm/include/llvm/Analysis/CallGraph.h:178
   /// and the call graph node being called.
-  using CallRecord = std::pair<WeakTrackingVH, CallGraphNode *>;
+  using CallRecord = std::pair<Optional<WeakTrackingVH>, CallGraphNode *>;
 
----------------
jdoerfert wrote:
> sdmitriev wrote:
> > jdoerfert wrote:
> > > This change is unrelated, right? Or did I miss where you differentiate between `None` and `nullptr`? If so, we should probably describe here what each means.
> > No, it is related. For ‘reference’ edges that are not associated with any call instruction CallRecord is created with the first field set to `None`, while real calls have instruction address there (please see lines 246-247 below). Therefore all real calls are expected to have a value in this field and it is not supposed to be `nullptr`. I will add a comment explaining this.
> I see. The comment helps but I might not have phrased my questions the right way. Didn't we already use an explicit `nullptr` as the "call" for the external call edges? That's why I thought you replaced `nullptr` with `none` now and asked if we distinguish between the two. It seems not.
> 
> The `nullptr` edge is for example in `llvm/test/Analysis/CallGraph/non-leaf-intrinsics.ll`
Ah, I see what you mean. So, you are basically asking why that change from `nullptr` to `None` is needed if `nullptr` is already used for representing external ‘call’ edges which are in fact ‘reference’ edges? Can’t we use explicit `nullptr` for “call” edges to callbacks?

You are right, in the current implementation explicit `nullptr` indeed represents a ‘reference’ edge, but such edge is allowed only for special “external” nodes which have `nullptr` function or for nodes representing function declarations. And a presence of an edge with `nullptr` call site for a defined function is considered as an error. CG SCC pass manager assumes that such situation can occur only if pass deleted a call instruction (which nulls WeakTrackingVH), but forgot to update the call graph with that change (please see `llvm/lib/Analysis/CallGraphSCCPass.cpp` below, lines starting from 236).

Therefore, use of explicit `nullptr` for an edge to callback function cannot be differentiated from the error case when pass forgot to update call graph after deleting call site. That is the reason why I made call site in the `CallRecord` optional. If `CallRecord` represents a real call it will always have value in the call site field (i.e. hasValue() will return `true`), and this value cannot be `nullptr` (otherwise it would be a stale edge). A ‘reference’ edge will not have a value in the call site field (hasValue() will be `false`). Does this sound reasonable?


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

https://reviews.llvm.org/D82572