[PATCH] D69456: [globalisel][docs] Rewrite the pipeline overview

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Thanks for writing this up! I just have a few suggestions and one small bug to point out (I can't figure out how to comment on a png file, so I'll write it here).

llvm/docs/GlobalISel/testing-pass-level.png: Between the Register Bank Selector and the Instruction Selector, it should be gMIR + MIR (right now it's just MIR).



================
Comment at: llvm/docs/GlobalISel/Pipeline.rst:25
+  analogous to SelectionDAGBuilder but builds a flavour of MIR called gMIR
+  instead of a specialized representation.
+
----------------
I think somewhere in this paragraph we should mention what gMIR stands for (just so we don't introduce unexplained acronyms - even if all the details are just a click away). 
Perhaps also a sentence or two explaining the exact relationship between MIR and gMIR would make the rest of the text easier to read (I'm thinking especially around lines 51-52) .


================
Comment at: llvm/docs/GlobalISel/Pipeline.rst:46
+legalizer to legalize an intrinsic directly to a target instruction. The
+concrete requirement is additional constraints are preserved after each of these
+passes.
----------------
Maybe "The concrete requirement is that the following constraints are preserved after each of these passes:"


================
Comment at: llvm/docs/GlobalISel/Pipeline.rst:52
+  The representation must be MIR after this pass. It will initially be gMIR but
+  will gradually transition to MIR.
 
----------------
You're using MIR both for "pure" MIR and for "MIRs in general". It's a bit confusing to read. I would suggest something along the lines of "The representation must be some kind of machine IR after this pass. It will initially be gMIR but will gradually transition to strict MIR throughout the rest of the pipeline." 


================
Comment at: llvm/docs/GlobalISel/Pipeline.rst:118
 
+It's possible to create an imaginary target such as in `LegalizerHelperTest.cpp <https://github.com/llvm/llvm-project/blob/93b29d3882baf7df42e4e9bc26b977b00373ef56/llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp#L28>`_
+and perform a single step of the algorithm and check the result. The MIR and
----------------
Just a quick question since I don't know how github links work: That's a link to a specific commit, right? (So the code and line number will stay the same forever).


================
Comment at: llvm/docs/GlobalISel/Pipeline.rst:121
+FileCheck directives can be embedded using strings so you still have access to
+the convenience available in llvm-lit.
----------------
I think this paragraph needs to be expanded a bit. Unless I missed it (entirely possible, I'm still waking up after the weekend), the text never mentions that legalization is performed in steps, so I think there should be a few words here about what the steps are and how they interact with each other (e.g. just by looking at that picture people might wonder if the steps can be run in any order).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69456