[PATCH] D61273: [globalisel] Update the legalizer documentation

[Jessica Paquette via Phabricator via llvm-commits]

paquette accepted this revision.
paquette added a comment.
This revision is now accepted and ready to land.

I think this looks good. I have a few stylistic suggestions, but I don't think they're worth holding up review over.

- Added some inline comments on document structure. I think that adding some more sections would make it easier to quickly search. I'm not sure if we can have that deep of nested sections though.
- I took this and copied it into Hemingwayapp (http://www.hemingwayapp.com) and found a few sentences were hard to read. I think it would be good to resolve some of those if possible? Of course, this is entirely stylistic preference. :)



================
Comment at: docs/GlobalISel.rst:343-345
+An API is broadly similar to SelectionDAG/TargetLowering is available but is not
+recommended as a more powerful API is available. The recommended API looks like
+this::
----------------
I think it would be better to mention the SelectionDAG/TargetLowering API after detailing the recommended method. Probably don't want people to think about the unrecommended API before thinking about the recommended API. ;)


================
Comment at: docs/GlobalISel.rst:358
+
+At the core of this ruleset is the ``LegalityQuery`` which describes the
+instruction. We use a description rather than instruction to both allow other
----------------
If `getActionDefinitionsBuilder` can have its own subsection, then I think this should too for symmetry's sake.


================
Comment at: docs/GlobalISel.rst:371
+
+The ``getActionDefinitionsBuilder`` function generates a ruleset for the given
+opcode(s) that rules can be added to. If multiple opcodes are given, they are
----------------
I think this is long enough that it deserves its own section? (Is it possible to have another subsection here? If not, meh, whatever.)


================
Comment at: docs/GlobalISel.rst:378-379
+
+There are various rule factories that append rules to a ruleset but they have a
+few outcomes in common:
+
----------------
It seems like this could have a subsection as well. Something like "Declaring Rules"?


================
Comment at: docs/GlobalISel.rst:381
+
+* legalIf(), legalFor(), etc. declare an instruction to be legal if the
+  predicate is satisfied.
----------------
Should `legalIf()`, `legalFor()` etc have code quotes?


================
Comment at: docs/GlobalISel.rst:425
+
+They also have predicates in common:
+
----------------
This could have a section just called "Predicates" or something?


================
Comment at: docs/GlobalISel.rst:450-451
+
+and finally there are some composite rules for common situations built out of
+the above facilities:
+
----------------
This could have a section called "Composite Rules"


================
Comment at: docs/GlobalISel.rst:468
+
+Predicates can also be combined using ``all(P0, P1, ...)``.
 
----------------
I think that if you add a "Predicates" section, this could be pulled into there


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61273