[PATCH] D21464: [PM] WIP: Introduce basic update capabilities to the new PM's CGSCC pass manager, including both plumbing and logic to handle function pass updates.
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 7 15:29:33 PDT 2016
I just wanted to post a brief note her, this is not intended to be a full
reply, but since you and a few others have asked I didn't want folks to not
know why there was silence...
On Thu, Jun 30, 2016 at 4:26 PM Xinliang David Li via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>> I would still really love to talk about any concerns or problems you see
>> with the design. Currently the one I'm thinking about the most is quadratic
>> behaviors, but if there are other concerns I'd like to discuss them.
> I may be repeating myself, but I would like to summarize my concerns and
> position here:
> 1. We have a goal to make the pass manager to use the new PM. There are
> many known/concrete performance improvements depending on that (there is
> possible short term solution that does not depend on new PM, but we've
> decided to make them depend on new PM switch anyway). Note the PM switch
> is already a very large task with potential risks, coupling that task with
> another huge change is not the right plan going forward. The right way is
> to make *minimal* changes enable new PM as soon as possible and make
> incremental improvement of other parts later. The longer we delay, the
> longer the community suffers.
> 2. Changes with this scale needs to be justified with concrete benefit. If
> there are anything real missing, we should have known them already.
> 3. With changes in core infrastructure like this, subtle bugs are likely
> to be very hard to debug. When a bug is introduced, there will no way for
> user to workaround it (such as by disabling a pass). This is reason why
> core piece like this needs to kept simple
FWIW, I think all of these comments apply much more to the overall effort
around the new pass manager than this patch. We can re-visit those
discussions if you want to do so, but I would like to separate out that
None of this is to say that we shouldn't try to keep the new pass manager
simple, and streamline the process of getting it in place where reasonable.
> 4. The patch introduces a new SCC formation algorithm (double layer with
> support for CG mutation on the fly), however design document on how this
> works is missing.
FWIW, this patch doesn't introduce any new algorithms like this. Those were
in previous patches to the LazyCallGraph.
That said, I'm very happy to try to provide a document around this. Much of
this was discussed extensively a long time ago when I originally worked on
> It needs to document
Yes, these discussions did not all materialize in a document. Some did, but
not enough. I'm working on these but it will take some time. Also, it is
somewhat orthogonal to *this* patch, except (as you point out) that it will
make review of this patch easier.
5. Another added complexity is that the SCC passes are now required to do
> the updates -- it seems pretty obscure to the pass writers.
This is already true though. PruneEH, the Inliner, and ArgumentPromotion
all update the call graph as they mutate the IR.
That said, it is a concern. With the old pass manager we skipped making
certain kinds of updates (SCC refinement is an example) and not being able
to skip those updates will add some complexity. However, I think we can
effectively provide utilities and common code to share that logic to the
extent possible. Some of this is already in LCG in fact.
I can imagine some of the code in this patch will get refactored in order
to be reused by other passes, but I didn't want to do that until I knew how
the other passes would actually use the code.
> 6. What is the plan to address the quadratic behavior?
I've added comments about this to the header. I don't have a single plan,
because there are several that seem very viable. One that I'd like to see
if it is sufficiently easy to do is to move to using the existing globals
graph as a form of indirection in the reference graph. That would cause the
graph to more closely match the IR and has the potential to open up some
really nice opportunities around GlobalOpt. But if that isn't easy to do,
there are several alternatives such as artificially building a layer of
indirection to cope with the potential for quadratic size.
> 7. Is this tested on large programs (e.g, in LTO mode)? How large is the
> RefSCC graph? How much memory overhead is expected? What is the compile
> time impact?
With any indirection around the reference graph to prevent duplicate edges
that don't exist in the IR, this is going to use much less memory than the
current call graph which is quite wasteful of memory. But I've not seen any
instances where the memory usage of the current call graph was a critical
part of our memory concerns, so I've mostly been focusing on removing the
one known source of quadratic growth (which I think is not scary here, but
if that's what is giving people pause, I'm happy to prioritize it).
> 8. This change makes new PM transition not NFC. It can potentially affect
> a lot of other people, e.g., introduced performance regression, correctness
> issue etc.
This is definitely true, but has actually been discussed before. I want to
point out that, the new PM is *already* not NFC. There are places where the
old pass manager has hacks that work around a lack of available analyses
(FunctionAttrs using AA for example).
I'm not sure making the pass manager NFC is a reasonable or interesting
goal. I'm much more interested in just making it *better*.
> So in short, my proposal is that we do minimal work in CGSCC pass manager
> to enable/unblock porting of inliner to the the new PM, and we can
> revisit/re-examine this patch in depth later when new PM is in place. Does
> it make sense?
I think the core issue is whether there is a simpler approach that actually
works and doesn't just introduce fragility into the system. I wasn't able
to find a good approach. Maybe Sean has, and if so, great. That, IMO,
doesn't mean we should stop working on the approach in this patch.
My plan going forward is to work on the design documents that are very
reasonable to explain how pieces of this work, and on looking at Sean's
approach to understand it.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits