[PATCH] D104059: [WIP][Attributor] Derive AACallEdges attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8177
+        if (Fn && !A.isAssumedDead(IRPosition::value(*Val), this, &LivenessAA))
+          AddCalledFunction(Fn);
+      }
----------------
kuter wrote:
> jdoerfert wrote:
> > Rather than asking for liveness, use a generic value traversal to visit all potential values of Val. Each of which is a potential callee. Also make sure to handle the case where a callee is not known, e.g., a function pointer call. We need to add them to the call graph as unknown callees too.
> Ok when I add the `genericValueTraversal` should we still keep the `getCallbackUses` function since it uses the callback metadata ?
> There is also a `callees` meta data that is ment to annotate a call, it gurantees that the called function is one of the functions it specifies.
> This seems useful for our purposes.  `getCallbackUses` doesn't use it.
> 
> I was planning to add a flag  to tell if there are any calls to a unknown function somethling like `bool hasUnknownCallee()` 
> 
> Also for us to be able to say that function `x` can't reach function `b`, we need to make sure that we know all functions that function `x` might ever call.  Meaning that if there are any functions that `x` can reach that has a unknown callee we can't tell for sure, right ?  
> 
I would have done getCallbackUses and then on the argument operands a genericValueTraversal. In the code above that means genericValueTraversal for `Val`. See also callees metadata below

If we have an unknown caller, we can reach any "visible" function. For now, you can assume any function but in reality you could exclude internal functions that don't have their address taken, for example.

So, how to use callees metadata is a good question. We want to expand what an AbstractCallSite is, I think. So first, an AbstractCallSite needs to get a new "kind", which is a direct call site with a "potential" callee. This is somewhat independent and we want to use it also as part of the checkForAllCallSites. To make this happen we'll need to either go through metadata uses (which is hard, IIRC) or use the AACallEdge in checkForAllCallSites. For the latter we also track in the AACallEdgeFunction node what all callers are, basically the callers should notify the AACallEdgeFunction of the Callee. If we do it that way, we can handle callee metadata here. I would suggest to have a new AbstractCallSite interface that allows you to iterate over a set of AbstractCallSites that a CallBase "exposes". For a direct call, that's just the direct callee. For a call with callees metadata, it will be one AbstractCallSite per callee node specified. And for all callbacks annotations, it would be one AbstractCallSite per callback annotation on the callee, basically line 8166-8168 above. This new interface would replace the getCallbackUses above and we could also (for now at least) ignore the genericValueTraversal thing. Certainly remove the isdead part.
Does this make sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104059