Page MenuHomePhabricator

[docs][NewPM] Add some instructions on how to invoke opt
ClosedPublic

Authored by aeubanks on Jun 23 2021, 1:17 PM.

Diff Detail

Event Timeline

aeubanks requested review of this revision.Jun 23 2021, 1:17 PM
aeubanks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2021, 1:17 PM

Thank you for this patch! It is very much welcome!

llvm/docs/NewPassManager.rst
390

Can you please document the relation/order between function,loop, and cgscc "adapters?" I think it would be good to provide more information about "adapters" here.

394–395

How does a developer know whether a pass is a module pass vs a function pass? Is there a way to find out? Can that be added to the documentation?

I assume some error message is produced when the proper "adapter" is left out? Perhaps having such an error message, with an explicit recommendation on what's going wrong, why, and how to resolve will make search engines able to index. That way, when a developer hits such an error, then searches for the error, they find this solution in the docs?

429

How does this differ from say:
-passes='function(my-function-analysis,my-module-pass'
?

aeubanks updated this revision to Diff 354111.Jun 23 2021, 5:02 PM
aeubanks marked an inline comment as done.

address comments

llvm/docs/NewPassManager.rst
394–395

it's part of the section below that prints all available passes, clarified there

429

-passes=my-function-analysis just isn't a thing
I've made it clearer what this actually does

nickdesaulniers accepted this revision.Jun 23 2021, 5:25 PM

This is incredibly helpful, thank you so much.

General feedback about NPM, completely orthogonal to this review.

I find the nesting a little confusing. Not the order, but the adapters. It seems they describe what they're wrapping, not what they're producing (unlike casting in C). For example, function(foo) wraps foo (which is a function pass) in a module pass adapter. It seems more intuitive IMO for the syntax to be module(foo), not function(foo). Does this have to do with the optional cgscc wrapper?

The help text from opt for incorrect nesting could be improved. Rather than feedback like unknown function pass 'no-op-module' for a valid pass name, but invalid nesting, it would be nice if opt could provide more helpful feedback in such a case like: 'no-op-module' is a function pass, not a module pass. Reinvoke with 'function(no-op-module)' in pass list. or something.

For a more concrete example,

$ opt -passes='adce,always-inline' foo.ll -S
opt: unknown function pass 'always-inline'

So this seems to be using the implicit module adapter you spoke of, but the error message is confusing. I assume that always-inline, a module level pass, is being wrapped in the function adapter since adce was first, and needed one.

$ opt -passes='function(adce),always-inline' foo.ll -S

would be the correct invocation. But I need to invoke opt --print-passes to find out what IR unit such a pass operates on. opt *knows* what IR unit each pass runs on (assuming it is a valid pass for some unit of IR); it should just tell me politely via a helpful diagnostic, or figure out what I meant, rather than error out and require me to look up and think about nesting adapters.

llvm/docs/NewPassManager.rst
423

maybe after "the IR unit" add "(module, cgscc, function, or loop)" to reiterate?

429

Sorry, I meant:

What is the difference between:
opt -passes='function(require<my-function-analysis>),my-module-pass' /tmp/a.ll -S
vs
opt -passes='function(my-function-analysis),my-module-pass' /tmp/a.ll -S

Wouldn't my-function-analysis run either way? If so, what's the point of require<>? If not, what difference does require<> imply?

This revision is now accepted and ready to land.Jun 23 2021, 5:25 PM
aeubanks updated this revision to Diff 354140.Jun 23 2021, 7:42 PM

clarify IR Unit

I see what you mean by module(func-pass) vs function(func-pass), but function(func-pass) is much more unambiguous. If there was a cgscc pass and a function pass with the same name it would be ambiguous.

As for a better error message, a similar issue is there, but of course most pass names are unique, so we could do something like what you propose. Or maybe just tack on a "perhaps the type of pass you're running is wrong".

llvm/docs/NewPassManager.rst
429

you don't directly run analyses, you can only run passes. require<foo> is basically just a pass that just queries for that analysis, putting it in the cache

This revision was landed with ongoing or failed builds.Jun 23 2021, 7:49 PM
This revision was automatically updated to reflect the committed changes.