Page MenuHomePhabricator

[docs][NewPM] Add docs for writing NPM passes
ClosedPublic

Authored by aeubanks on Sep 1 2020, 1:43 PM.

Details

Summary

As to not conflict with the legacy PM example passes under
llvm/lib/Transforms/Hello, this is under HelloNew. This makes the
CMakeLists.txt and general directory structure less confusing for people
following the example.

Much of the doc structure was taken from WritinAnLLVMPass.rst.

This adds a HelloWorld pass which simply prints out each function name.

More will follow after this, e.g. passes over different units of IR, analyses.
https://llvm.org/docs/WritingAnLLVMPass.html contains a lot more.

Diff Detail

Event Timeline

aeubanks created this revision.Sep 1 2020, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 1:43 PM
aeubanks requested review of this revision.Sep 1 2020, 1:43 PM
aeubanks updated this revision to Diff 289281.Sep 1 2020, 1:59 PM

more updates

aeubanks edited the summary of this revision. (Show Details)Sep 1 2020, 1:59 PM
aeubanks edited the summary of this revision. (Show Details)Sep 1 2020, 2:02 PM
aeubanks added reviewers: ychen, asbirlea, hans.
foad added a subscriber: foad.Sep 1 2020, 11:34 PM
ychen added a comment.Sep 2 2020, 11:14 AM

Thanks for getting this started.

llvm/docs/WritingAnLLVMNewPMPass.rst
20

Not an English grammar expert. But it feels to me the was should is?

22

As mentioned in the comments on top of PassManager.h, I think we could say there is no interface for pass since the NPM is adopting concept-based polymorphism where not using inheritance is a goal. Maybe add a reference to https://sean-parent.stlab.cc/papers-and-presentations/#value-semantics-and-concept-based-polymorphism

48

Do we need this "(just `opt` is necessary for now)"? If I'm new to LLVM, I would find this a little bit distracting. It does not hurt anything though.

91

Could we delete this?

98

#endif // LLVM_TRANSFORMS_HELLO_HELLOWORLD_H

100

Extra comma. Or if we delete the ctor, we could also delete the wording here?

114

Is the ... here intentional?

Looking at https://llvm.org/docs/WritingAnLLVMPass.html makes me think it was a typo.

133

ditto

146

ditto

llvm/include/llvm/Transforms/HelloNew/HelloWorld.h
10

LLVM_TRANSFORMS_HELLONEW_HELLOWORLD_H

aeubanks updated this revision to Diff 289541.Sep 2 2020, 12:29 PM
aeubanks marked 5 inline comments as done.

address review comments

llvm/docs/WritingAnLLVMNewPMPass.rst
20

I was trying to go for the angle of "the legacy PM is in the past", but I guess that's not true yet, so "is" makes more sense.

22

Clarified a bit, mentioned comments in PassManager.h rather than link to that since there's more that we're missing anyway.

48

Done. Also linked to GettingStarted.

114

I think it's meant to show that the comment is a continuation of before the code snippet which makes sense to me.
I changed a bit of the wording in a couple places, let me know if it's still weird.

ychen accepted this revision.Sep 2 2020, 12:46 PM

One more comment. Please let another reviewer ack.

llvm/docs/UserGuides.rst
57

This needs a :doc: entry below.

This revision is now accepted and ready to land.Sep 2 2020, 12:46 PM
aeubanks updated this revision to Diff 289599.Sep 2 2020, 4:05 PM

One more place in UserGuides.rst

asbirlea accepted this revision.Sep 14 2020, 12:56 PM

Thank you adding this!

llvm/docs/WritingAnLLVMNewPMPass.rst
78

s/should/should be
or
s/containing/contain

aeubanks updated this revision to Diff 291662.Sep 14 2020, 1:16 PM

rebase
containing -> contain

This revision was landed with ongoing or failed builds.Sep 14 2020, 1:26 PM
This revision was automatically updated to reflect the committed changes.

This commit seems to be breaking the build with -DBUILD_SHARED_LIBS=TRUE. Could you please have a quick look? Thanks!

This commit seems to be breaking the build with -DBUILD_SHARED_LIBS=TRUE. Could you please have a quick look? Thanks!

Taking a look

This commit seems to be breaking the build with -DBUILD_SHARED_LIBS=TRUE. Could you please have a quick look? Thanks!

Taking a look

Reverted, will fix and push again