[PATCH] D86979: [docs][NewPM] Add docs for writing NPM passes

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/docs/WritingAnLLVMNewPMPass.rst:19
+
+Unlike passes under the legacy pass manager where the pass interface was
+defined via inheritance, passes under the new pass manager rely on template
----------------
ychen wrote:
> Not an English grammar expert. But it feels to me the `was` should `is`?
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.


================
Comment at: llvm/docs/WritingAnLLVMNewPMPass.rst:21
+defined via inheritance, passes under the new pass manager rely on template
+metaprogramming. All LLVM passes inherit from the CRTP mix-in
+``PassInfoMixin<PassT>``. The pass should have a ``run()`` method which
----------------
ychen wrote:
> 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
Clarified a bit, mentioned comments in PassManager.h rather than link to that since there's more that we're missing anyway.


================
Comment at: llvm/docs/WritingAnLLVMNewPMPass.rst:47
+
+First, configure and build LLVM (just ``opt`` is necessary for now).
+
----------------
ychen wrote:
> 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.
Done. Also linked to GettingStarted.


================
Comment at: llvm/docs/WritingAnLLVMNewPMPass.rst:113
+
+... to include the header file we just created.
+
----------------
ychen wrote:
> Is the ... here intentional?
> 
> Looking at https://llvm.org/docs/WritingAnLLVMPass.html makes me think it was a typo.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86979