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

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 10 10:12:41 PST 2023


jdoerfert added a comment.

In D141190#4039570 <https://reviews.llvm.org/D141190#4039570>, @nikic wrote:

> 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.

You are right, we don't call the intrinsic but the external node. That is even worse. Why do we have the intrinsics in the CG in the first place (among other things) if we don't connect them at all?

>> 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.

FWIW, an intrinsic is (in our encoding) a call. In our model it will execute some code.
Given a call to an intrinsic `llvm.abc [attrs]` and a function declaration `abc [attrs]`, all we know is `[attrs]`.
The details, e.g., "call like", is part of `[attrs]` in both cases. If we stop distinguishing them above we will remove bugs and special handling code.

> 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.

Will be addressed before I commit this.

---

I'm assuming with the fix to GlobalsAA you are OK with this now. I'll commit at some point today.



================
Comment at: llvm/lib/Analysis/GlobalsModRef.cpp:609
-          continue;
-        }
-
----------------
nikic wrote:
> 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?
Agreed, we should do continue. I'll fix that pre-commit.

For intrinsics and declarations the change didn't matter since the code below should just read the attributes again. That's all we knew about them to begin with.
For definitions we could know more then the attributes, but that's somewhat unlikely at this stage since GlobalsAA doesn't really do much deduction, just summarizes the body. Assuming the attributes already summarize the body properly, there would not be a change in the MRI computed for the function.
Still worth skipping them.


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

https://reviews.llvm.org/D141190



More information about the llvm-commits mailing list