[PATCH] D141190: [CallGraph][FIX] Ensure generic intrinsics are represented in the CG

[Nikita Popov via Phabricator via llvm-commits]

nikic added a reviewer: aeubanks.
nikic added a comment.

In D141190#4037719 <https://reviews.llvm.org/D141190#4037719>, @jdoerfert wrote:

> Right now, we add most intrinsics to the CG but only add *incoming* egdges for isLeaf intrinsics and never outgoing edges. Which means our CG is totally broken and mostly useless wrt. intrinsics.
> Even the leaf ones are not represented reasonably because they are called but themselves call nothing (according to the CG). That works for some other magic reasons.

I don't think this is quite right: For !isLeaf intrinsics an edge to CallsExternalNode is added, not to the intrinsic. This seems consistent with the current model where intrinsics themselves are not part of the call graph, so only the call that may be done by the intrinsic is represented in the CG.

> With this patch we add incoming and outgoing edges to intrinsic nodes where they make sense. So if the node is called, it has an incoming edge, if it can call back, it has an outgoing edge.
> These are two separate changes and both needed to make the CG sane.
>
> TLDR; Intrinsics are not special in any way. We should treat them like function declarations, which this patch does.

I don't really agree that intrinsics aren't special. Intrinsics are basically an extensible mechanism for custom instructions. //Some// intrinsics may be call like (those that are !nocallback), some intrinsics may be load/store like (those with memory effects) and some may be more like arithmetic instructions (most intrinsics). With that in mind, I don't think it's unreasonable for a call graph to only model the "call like" intrinsics.

All that being said, as we do happen to encode intrinsics as calls, it's also not unreasonable to model all intrinsics in the call graph either. And it does look like this makes the implementation simpler overall, so that seems fine as long as it doesn't regress optimization. But see the note inline -- I don't think your change to GlobalsAA is right.



================
Comment at: llvm/lib/Analysis/GlobalsModRef.cpp:609
-          continue;
-        }
-
----------------
This change doesn't look right to me: Now you're going to handle intrinsic calls both via the call graph, and via the crude code directly below. After your changes, shouldn't be be doing a continue on all calls instead? Why is this not causing any test failures -- are we missing coverage? Or am I completely misunderstanding something here?


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

https://reviews.llvm.org/D141190