<div dir="ltr"><div>I'm missing a lot of the context for the initial implementation of the NPM CGSCC infra so I may be wrong in some aspects, but my initial thoughts are below</div><div dir="ltr"><div dir="ltr"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 27, 2020 at 10:49 PM Johannes Doerfert <<a href="mailto:johannesdoerfert@gmail.com" target="_blank">johannesdoerfert@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hey,<br>
<br>
Recently I ran into multiple problems with the new PM CGSCC Attributor pass.<br>
Instead of going into details now, I want to first ask what we<br>
want/expect to work when it comes to CGSCC passes (in the new PM).<br>
<br>
What I was hoping to (eventually) do are the following things:<br>
<br>
- Internalize functions, that is, replace all uses of<br>
`define weak @foo() {}`<br>
with<br>
`define private @foo.internal() {}`<br>
while keeping @foo around.<br></blockquote><div>I can see issues where foo.internal() is created (say when visiting foo()), but the CGSCC pass manager doesn't visit it, and then later on when a caller of foo() now calls foo.internal(), the CGSCC contract is broken since foo.internal() hasn't been visited yet.</div><div>Pretty sure that modifying all callers of foo() when splitting foo() is bad since you'd be modifying a SCC further up.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Delete functions, both in the current SCC but also children SCCs, e.g.,<br>
```<br>
static void bar() { ... }<br>
static void baz() { bar(); }<br>
void quack() { if (/* something that is basically */ false) baz(); }<br>
```<br>
We visit the singleton SCCs with bar, baz, then quack only to realize<br>
when we optimize quack that baz and bar are dead and can be removed.<br></blockquote><div>The inliner already deletes non-recursive dead functions, so I think this should already work. (Although looking through the code, the inliner wouldn't delete bar in your case which seems like an oversight)</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Delete multiple functions from the current SCC, this is already<br>
breaking with our current (inliner-centric) update methods (AFAIKT).<br></blockquote><div>To clarify, deleting potentially mutually recursive functions in the current SCC? That seems reasonable, maybe by making the existing LazyCallGraph::removeDeadFunction() more generic.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Add/Remove/Modify globals variables. (Not related to the call graph,<br>
but I wanted to mention it anyway.)<br></blockquote><div>Not sure that modifying globals makes sense in a CGSCC pass. Maybe if the global were only used by functions in the current SCC. Any specific examples?</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Outline parts of a function in the current SCC into a new function<br>
that will then either become part of the current SCC (even if that is<br>
an overapproximation) or a child SCC.<br></blockquote><div>I've actually been trying to work on this to make coroutines work under the NPM. For an example see <a href="https://reviews.llvm.org/D88714" target="_blank">https://reviews.llvm.org/D88714</a>. I've been meaning to ask for more specifics on exactly what coroutines do to the call graph to better understand what a proper solution would look like.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Delete calls to any function.<br>
<br>
There might be more but this is all I remember right now. If it turns<br>
out too many things cannot be supported, it's unclear if the module<br>
Attributor pass is not going to be preferable, especially since we can<br>
parallelize the pass itself more easily (I think) than multiple<br>
concurrent CGSCC passes.<br></blockquote><div>I have a feeling that concurrency at the pass manager level is a long way off.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I'd appreciate any thoughts on this :)<br></blockquote><div>At a high level, does the attributor work on SCCs? It seems that it works on the call graph, but not in a bottom-up way, making it different from typical CGSCC passes. That makes me think it shouldn't be working with the CGSCC infra. Did you want to run it alongside the inliner/other CGSCC passes?</div></div></div>
</div>
</div>