Move mandatory inlining cleanup to mandatory combine

[zoecarver]

This patch makes MandatoryInlining a SILModuleTransform instead of a SILFunctionTransform. It also moves the function removal cleanup to MandatoryCombine so that it will be run on every function. This is better because then we don't have to iterate over every function in a module twice in MandatoryInlining. And now, both can remain a SILFunctionTransform. It also has the benefit of allowing MandatoryCombine to exit early sometimes.

The successor to #28394.

cc @gottesmm

[gottesmm]

This is the wrong way to do this. What we want to do is move stuff into the mandatory combiner since it runs after diagnostics. That way we can make it easy for SourceKit to not run this sort of code.

[zoecarver]

I re-wrote this patch fixing most of the issues that you brought up. I removed changes to MandatoryCombine. I'll either add those to this patch later or add them in another patch.

The basic flow of this pass is now:

• Collect apply instructions.
• Try to devitalize applys.
• Iterate over every apply and try to inline it. Otherwise, continue.
• Add the apply's callee to the pass manager worklist so it will also try to be inlined (instead of trying to inline it recursively).
• If any functions were inlined, add them to the worklist as well so the now-dead functions can be removed.

The recursive inlining is now removed. All mandatory inlining tests pass (assuming I ran the right tests).

There are still a few things I want to clean up (see TODOs). I'll probably fix those in a different patch, though.

[gottesmm]

Some initial comments. I want to read this code in more detail before it lands.

[gottesmm]

Why do you need to use the dominator info here?

[zoecarver]

We don't. I'll remove it.

[zoecarver]

Hmm. To be honest, I'm not quite sure why reading the blocks in dominance order fixes this, but, when inlining an auto closure with -enable-ownership-stripping-after-serialization, the verifier fails because convert_escape_to_noescape [not_guaranteed] not allowed after mandatory passes. Using dominance order fixes that.

Edit: see test_chained_short_circuit for example.

[gottesmm]

This should not be necessary. Please remove this.

[zoecarver]

testOriginalProtocolImpl doesn't inline cFuncOriginal in mandatory_conditional_compile_out_using_optionals.swift (among others) when this pass isn't run for Onone.

[gottesmm]

So I was thinking about this a little bit. We need to do this differently/split this up. Here is my suggestion:

1. Why don't we create a separate PR that just moves cleanup code (and leaves everything else the same). This would mean eliminating the code in cleanup callee and moving that to the Mandatory Combiner. No passes should be moved around, it should just require having the mandatory inliner not cleaning up after itself and leaving that later for the mandatory combiner.

2. There is more complexity here around the pass manager. I can explain it in more detail offline, but I think that this may be a bit more complex than a simple cleanup. Maybe we do 1 in the short term and then circle back around to this?

[zoecarver]

@gottesmm, that sounds good to me.

I'll look into making a cleanup patch. I'm not 100% sure the cleanup done here is necessary, though. I think the mandatory combiner might already handle this. I'll double-check and make a patch if not.

I would also be interested to learn more about the pass manager. Feel free to reach out to me whenever (no rush, though, I'm sure you're busy).

[zoecarver]

Closing for now. Might reopen later.