Page MenuHomePhabricator

[globalisel][docs] Rewrite the pipeline overview
ClosedPublic

Authored by dsanders on Oct 25 2019, 6:18 PM.

Details

Summary

Rewrite the pipeline overview to be more focused on the structure and
flexibility as well as highlight the increased usefulness of
MachineVerifier and increased testability resulting from the smaller
incremental passes approach.

The diagrams are lifted from the slides for the LLVMDev 2019 talk
'Generating Optimized Code with GlobalISel' and adapted to be readable on
the white background used in the docs.

Diff Detail

Event Timeline

dsanders created this revision.Oct 25 2019, 6:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2019, 6:18 PM
rovka added a comment.Oct 28 2019, 2:14 AM

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

llvm/docs/GlobalISel/Pipeline.rst
25

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

59

Maybe "The concrete requirement is that the following constraints are preserved after each of these passes:"

63

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

134

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

137

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

dsanders marked 7 inline comments as done.Oct 28 2019, 11:23 AM

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

llvm/docs/GlobalISel/Pipeline.rst
59

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

63

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?

134

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

137

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

dsanders updated this revision to Diff 226715.Oct 28 2019, 11:24 AM
dsanders marked 2 inline comments as done.

Updated for review comments (except the picture, that will follow shortly)

dsanders updated this revision to Diff 226716.Oct 28 2019, 11:26 AM

Fix the image too

volkan accepted this revision.Oct 28 2019, 2:37 PM

LGTM with a nit.

llvm/docs/GlobalISel/Pipeline.rst
82

It would be nice to describe combiner too like the other passes.

This revision is now accepted and ready to land.Oct 28 2019, 2:37 PM
rovka accepted this revision.Oct 29 2019, 2:11 AM

Thanks, looks great!

This revision was automatically updated to reflect the committed changes.
dsanders marked 2 inline comments as done.Oct 29 2019, 11:54 AM

Thanks. llvm.org doesn't seem to be have the previous changes yet so I've asked on llvm-dev about that. Maybe the update mechanism is broken

llvm/docs/GlobalISel/Pipeline.rst
82

I added a brief mention. I'll expand on it when I get to that pass