[PATCH] [Requested Changes To] D24226: [PM] Provide an initial, minimal port of the inliner to the new pass manager.

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Most of the comments down below are stylistic issues, except:  `InlinerPass::run` in this patch does not handle the full generality of `InlineFunction`.  That is the most important thing that needs to be addressed.



> CGSCCPassManager.h:527
> +std::pair<LazyCallGraph::RefSCC *, LazyCallGraph::SCC *>
> +updateCGAndAnalysisManagerForDeadEdge(
> +    LazyCallGraph &G, LazyCallGraph::RefSCC *RC, LazyCallGraph::SCC *C,

Please document the `RC` and `C` parameters-- it isn't obvious what they represent.  Are they `SourceRC` and `SourceC`?

Also, the return value needs to be documented.

> CGSCCPassManager.cpp:144
> +  SCC &TargetC = *G.lookupSCC(TargetN);
> +  RefSCC &TargetRC = TargetC.getOuterRefSCC();
> +

If `RC` and `C` are the source `RefSCC` and `SCC`, then please add an assert to make that obvious.

> CGSCCPassManager.cpp:163
> +  if (!NewRefSCCs.empty()) {
> +    // Note that we don't bother to invalidate analyses as ref-edge
> +    // connectivity is not really observable in any way and is intended

This should be mentioned on the declaration of `RefSCC` itself.

> CGSCCPassManager.cpp:182
> +    assert(G.lookupRefSCC(SourceN) == RC && "Failed to update current RefSCC!");
> +    for (RefSCC *NewRC : reverse(NewRefSCCs))
> +      if (NewRC != RC) {

I'd just assert that the first element of `NewRefSCCs` is `RC` and then skip the first element in the loop.

Checking `NewRC != RC` in every iteration may let a bug slip through undetected.

> CGSCCPassManager.cpp:393
> +
> +  if (N.begin() == N.end())
> +    // Can't remove edges that don't exist!

Add a `Node::empty()`?

> CGSCCPassManager.cpp:421
> +      if (Function *Callee = CS.getCalledFunction())
> +        if (Visited.insert(Callee).second && !Callee->isDeclaration())
> +          // Actually do the work here of erasing the callee from the dead

What is the extra check on `Visited.insert(Callee).second` getting you here?  Why not just try to `DeadCallTargets.erase(Callee)` directly?

> CGSCCPassManager.cpp:429
> +
> +  // Now walk all references. We do this incrementally and with a fresh walk of
> +  // instructions to allow us to end both of these walks early if we run out of

I don't buy this reason for doing two walks.  :)

I think you should be able to do a combined walk and break out if `DeadRefTargets.empty() && DeadCallTargets.empty()`, and predicate the `DeadCallTargets` on `!DeadCallTargets.empty()` and corresponding for `DeadRefTargets`.  We'll have the overhead of repeated calls to `SmallPtrSet::empty()` but that sounds cheaper than walking all instructions twice.

However, walking twice looks simpler.  If you'd rather keep it this way to make things readable, I can buy that. :)

> CGSCCPassManager.cpp:454
> +  // to identify demoted edges as we have enough information already.
> +  SmallVector<PointerIntPair<Node *, 1, Edge::Kind>, 4> DeadTargets;
> +  SmallVector<Node *, 4> DemotedRefTargets;

I'd s/`DeadTargets`/`DeadEdges`/

> CGSCCPassManager.cpp:455
> +  SmallVector<PointerIntPair<Node *, 1, Edge::Kind>, 4> DeadTargets;
> +  SmallVector<Node *, 4> DemotedRefTargets;
> +  for (Edge &E : N) {

I'd s/`DemotedRefTargets`/`DemotedCallTargets` (i.e. these were call targets that were demoted).  Alternatively `TargetsDemotedToRef` (actually, this one sounds better).

> eraman wrote in Inliner.cpp:848
> If only one block is cloned into the caller, it is spliced into the block containing the call instruction. This logic to walk the newly added basic blocks does not work in that case.

Looks like this one wasn't addressed.

> Inliner.cpp:861
> +                // into the graph. This will even skip already present edges.
> +                RC->insertTrivialCallEdge(N, *CG.lookup(*NewCallee));
> +              }

As mentioned in `D24226`, `InlineFunction` can insert non-trivial call edges (by promoting ref edges), causing SCCs to be formed.

https://reviews.llvm.org/D24226