[PATCH] D85838: New TableGen Programmer's Reference document

Sat Aug 15 14:50:49 PDT 2020

johnwbyrd added a comment.

I'm a little disappointed that this document does not describe the *intention* of TableGen, what exactly its output files are, their specific purposes, or where they are generally used in LLVM.  It's possible that this information is ill suited for a reference manual, and belongs in a guide instead.  Regardless, despite this draft's limitations, it's an improvement over the current state of things, and with a few improvements I'll happily sign off on it.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:13-15
+The purpose of TableGen is to generate complex output files based on
+information from input files that are significantly easier to code than the
+output files would be, and also easier to maintain and modify over time. The
----------------
This doesn't distinguish the purpose of TableGen from any other program that processes input to output.  Why does TableGen need to exist?  Who should know how to work with it?  What are its areas of responsibility?

================
Comment at: llvm/docs/TableGen/ProgRef.rst:29-30
+target-independent code generator. See :doc:`TableGen Backends <./BackEnds>` for
+a description of the LLVM TableGen backends. Here are a few of the things
+backends can do.
+
----------------
Why just a few of the things?  Why not all of the things?  This is a reference document.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:50-51
+TableGen source files contain two primary items: *abstract records* and
+*concrete records*. In this and other TableGen documents, abstract records
+are called *classes.* In addition, concrete records are usually just called
+records, although sometimes the term *record* refers to both classes and
----------------
Differentiate between TableGen classes and C++ classes, please.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:63
+A backend processes some subset of the concrete records built by the
+TableGen parser and emits the output files. In a complex use case such as
+the LLVM code generator, there can be many concrete records and some of them
----------------
What kind of output files? This is way too vague.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:112
+
+TableGen supports a simple preprocessor that can be used to conditionalize portions of ``.td`` files. See `Preprocessing Facilities`_ for more information.
+
----------------
Reformat to 80 columns.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:264-266
+    The ``bits`` type is a fixed-size integer of arbitrary length *n* that is
+    treated as individual bits.  This type
+    is useful because it can handle some bits being defined while others are undefined.
----------------
This doesn't describe why "bits" fields are useful, how they are stored, or generally what their purpose is in code.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:274
+``dag``
+    This type represents a nestable directed acyclic graph (DAG) of elements.
+
----------------
Why are DAGs important to TableGen?

================
Comment at: llvm/docs/TableGen/ProgRef.rst:276
+
+.. The dag type needs more explanation.
+
----------------
Yes, and please don't submit this rev without adding some.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:288-289
+
+To date, these types have been sufficient for describing the data that
+TableGen generates, but it is straightforward to extend this list if needed.
+
----------------
Not needed as part of this reference.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:533
+
+See `Interlude 1: Class and Record Examples`_ for examples.
+
----------------
Example, not "interlude."

================
Comment at: llvm/docs/TableGen/ProgRef.rst:589
+
+A ``def`` statement defines a record. 
+
----------------
Redundant.  What, specifically, does it mean to "define" a record?

================
Comment at: llvm/docs/TableGen/ProgRef.rst:629-633
+See `Interlude 1: Class and Record Examples`_ for examples.
+
+
+Interlude 1: Class and record examples
+--------------------------------------
----------------
"Interlude"?  Unnecessarily pretentious.  This is an example.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:748-750
+
+``let`` --- override fields in classes or records
+-------------------------------------------------
----------------
This concept is good, but it doesn't follow the parallel structure of headings in the rest of the document.  The heading is the keyword.  Use text to define the concept.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:838
+
+See `Interlude 2: Multiclass and Defm Examples`_ for examples.
+
----------------
Example, not an interlude.  Also, the example should go here, not after defm.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:844-845
+
+Once multiclasses have been defined, you use the ``defm`` statement to
+instantiate the multiple records specified in those multiclasses.
+
----------------
The records are not "specified" in the multiclasses.  Multiclasses permit instantiation of multiple similar TableGen definitions, with a single "defm" statement.  Not the same thing -- please reword.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:855-856
+
+This statement instantiates all the records defined in all the specified
+multiclasses. Those records also receive the fields defined in the specified
+regular classes. This is useful for adding a common set of fields to all the
----------------
The records are not defined in the multiclasses.  They are defined by the defm statement itself.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:887-888
+
+Interlude 2: Multiclass and defm examples
+-----------------------------------------
+
----------------
Use the term example, please.  There is no other place in the LLVM documentation that mentions "interludes."

================
Comment at: llvm/docs/TableGen/ProgRef.rst:895-912
+.. code-block:: text
+
+  def ops;
+  def GPR;
+  def Imm;
+  class inst <int opc, string asmstr, dag operandlist>;
+
----------------
I am seeing now that the use of "interlude" or "example" is inconsistent.  Some code is marked as an example and some is not, without clear logic as to which is which.  I suggest removing the numbering from all interludes/examples, and linking to them from the body text if the example does not immediately follow the code that describes it.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:949-950
+
+A ``defm`` can be used in a multiclass to gain multiple levels of record
+instantiations.
+
----------------
Defm doesn't allow you to "gain multiple levels."  Reword to be more technically accurate please.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:1145
+
+Here is a simple example.
+
----------------
Unnecessary -- your other examples merely follow without being introduced.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:1211-1212
+
+How records are built
+---------------------
+
----------------
This section looks new.  Useful info.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:1219
+
+2. Parse the superclasses from left to right, visiting each superclass's parent
+   classes in depth-first order.
----------------
What does "left to right" mean in this context?  The order in which the superclasses were instantiated?  Or from child to parent order?  Word more precisely please.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:1283-1284
+
+The preprocessor embedded in TableGen is intended only for simple
+conditional compilation. It supports the following directives, which are
+specified somewhat informally.
----------------
This section looks new too.  Good job.

================
Comment at: llvm/docs/TableGen/ProgRef.rst:1562
+
+The Intel x86 processors are complex. The following output from TableGen shows
+the record that is created to represent the 32-bit register-to-register ADD
----------------
Not really relevant.  All generated instruction records tend to look more or less this complex on all platforms.

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

https://reviews.llvm.org/D85838