[PATCH] D108298: [WIP][NPM] Print '-passes' compatible string for built pipeline.

Thu Aug 19 02:09:41 PDT 2021

markus added a comment.

In D108298#2953266 <https://reviews.llvm.org/D108298#2953266>, @aeubanks wrote:

> The point of this is to reduce pass pipelines right? And this is only relevant when there's state not captured in the IR.

Yes, that and being able to see what passes are included in a pipeline without having to run it on any special input. Keeping in mind that different targets have their own extension points so pipelines do differ depending on the target.

> Another approach would be to bisect where some state becomes corrupted. We can split the pipeline into two, where we first run the first set of passes, dump the IR, then only run the second set of passes on the dumped IR. Something similar to opt-bisect. This should be much less invasive than this proposed patch.

Perhaps, I don't know how that would work though so I cannot comment on how invasive it would be nor if it would work sufficiently well. In general though if automation fails it is always useful to be able to step in and do things manually so automatic bisection would not replace that.

> And I have no proof of this, but it seems like we wouldn't be able to reduce a lot of the pipeline, since earlier passes, especially when running on unoptimized IR, will make a lot of necessary changes to trigger issues.

Not sure about that. If the IR is already reduced on a full pipeline (with e.g. llvm-reduce) then it could be that a large number of the pipeline passes are effectively a no-op on that IR.

> If we do go down this proposed patch's route, we need to decide to what level do we want to make a pipeline completely representable as a string. Looking at PassBuilder.cpp, I don't think it's feasible to represent literally every possible pipeline as a string.
> The legacy `-debug-pass=Arguments` decides to give up on handling pass params. It'll be a tradeoff of representing more vs more code changes/invasiveness. If we want to support pass params of any kind, each pass will have to map its params to a string (i.e. it can't be a static method). I'm not sure I really like adding more to `PassInfoMixin`.

I don't know what is the best approach here. Seems some sort of decision needs to be taken.

> Currently we just have a mapping of names to a pass. This patch is basically trying to add a map going back the other way and also trying to keep it in sync with the PassRegistry.def map. It seems wrong to me... I never liked `PassInstrumentationCallbacks::getPassNameForClassName()`, I only added it to support `--print-before/after`.

Not sure that I understand this. There already is a mapping from ClassName to pass-name and I don't see how this patch is doing anything differently. Or is it so that this mapping should not exist in the first place.

> If we're not going to use pass instrumentation, we shouldn't use `PassInstrumentationCallbacks` to get the pass name. We should do something similar where we gather up class -> pass names by including PassRegistry.def.

For sure, that could be refactored and the map could be placed elsewhere if we do agree that such a map should exist.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108298