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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked 7 inline comments as done.
dsanders added a comment.

In D69456#1723011 <https://reviews.llvm.org/D69456#1723011>, @rovka wrote:

> 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).


Well spotted. These are images straight from our LLVMDev Keynote slides so I guess I should fix that while I'm at it. Hopefully I can sneak the fix in before Tanya uploads them :-)



================
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.
----------------
rovka wrote:
> Maybe "The concrete requirement is that the following constraints are preserved after each of these passes:"
In addition to that, I also expanded on the bit about flexibility and explicitly stated the general point that doing things early is ok rather than just hinting at it with the example


================
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.
 
----------------
rovka wrote:
> 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." 
Good point. We never really came up with a term for the original MIR to contrast it with gMIR.

I've tried to dodge the issue here by using `gMIR, MIR, or a mixture of the two`. Does it read better?


================
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
----------------
rovka wrote:
> 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).
Yes, that's right. I got it from the 'Copy Permalink' option that's in the '...' button to the left of the marked line.

I just found out that you can do line ranges too so the link now highlights the whole function


================
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.
----------------
rovka wrote:
> 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).
I don't really want to go into the details of the passes here so I've kept that intentionally abstract. I think the place to talk about the details is the individual pass pages. The main point I want to make here is that it's possible to go much deeper into testing than SelectionDAG could


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69456