Page MenuHomePhabricator

New TableGen Programmer's Reference document
ClosedPublic

Authored by Paul-C-Anagnostopoulos on Aug 12 2020, 7:38 AM.

Details

Summary

This new TableGen Programmer's Reference document replaces the current Language Introduction and Language Reference documents. It brings all the TableGen reference information into one document.

As an experiment, I numbered the sections in the document. See what you think about that.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Paul-C-Anagnostopoulos requested review of this revision.Aug 12 2020, 7:38 AM
lattner accepted this revision.Aug 13 2020, 4:27 PM

This is really fantastic work, thank you for doing this!

llvm/docs/TableGen/ProgRef.rst
14

"complete detail" might be a stretch :-). People will always want more, and this will replace the existing documentation - you don't need to reference the old thing.

The first paragraph of the introduction should be an introduction to tablegen, something closer to your second paragraph.

63–64

I'd phrase it slightly differently maybe something like:

It is typical for abstract records to have many missing fields which are then filled in when they are instantiated into a concrete record, but even a concrete record may have fields with the value of ?.

66

I'd replace "In a complex program such as LLVM," with "In a complex use case like the LLVM code generator,"

93
101

I'd mention the preprocessor in this section

216

I'd mention #ifndef/define as well since this is an overview

310

It would be nice to deprecate and remove this at some point, .. is much nicer :-)

This revision is now accepted and ready to land.Aug 13 2020, 4:27 PM

Thanks! I quite enjoyed writing it. I will incorporate your comments and submit a new patch.

I did not yet attempt to find all the references to the existing two TableGen documents and change those references. I figure I would do that as a separate patch later. Does that make sense?

Could you comment on the '..' patch? https://reviews.llvm.org/D85585

Yep, as long as you're committed to doing it, I'm perfectly fine with doing that in a followup patch. You might want to land this at one of the old URLs to make that easier though.

Thank you again for driving this!

madhur13490 added inline comments.
llvm/docs/TableGen/ProgRef.rst
263

I think this is the most complex type. May be its worthwhile to get some more info on this document? In general I would like if we can capture its syntax, explanations on keywords like ops, ins, outs and some examples on this.

Yep, as long as you're committed to doing it, I'm perfectly fine with doing that in a followup patch. You might want to land this at one of the old URLs to make that easier though.

I think to do this, the file would have to be named the same as one of the old files. I gave the document a new title to avoid any conflicts with the old names. I don't mind hunting up all the existing references and changing them to point to the new document.

I will add a section on the dag type. However, I will do that as a future update, since it will take awhile to write the new section. In particular, I have to see how much is written elsewhere and include references as appropriate.

Paul-C-Anagnostopoulos marked 6 inline comments as done.

Here is the final document with Chris Lattner's suggestions. I have also included index.rst, which now refers to this document rather than the two old ones.

Could someone commit this for me, since I do not have commit privilege?

Sorry, I noticed one typo just after I submitted the previous revised patch. It is corrected in this patch.

Hi Paul,

Please take a look at the LLVM Developer Policy and follow the instructions on commit access. :-)

Hi Paul,

Please take a look at the LLVM Developer Policy and follow the instructions on commit access. :-)

I appreciate the offer, but, as Paul Robinson will attest, I need more practice with Git. I'm reading a book on it for the third time, which I hope will do the trick.

I feel comfortable messing around with my own repository, but I'd appreciate someone else committing my next couple of patches.

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.

llvm/docs/TableGen/ProgRef.rst
14–16

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?

30–31

Why just a few of the things? Why not all of the things? This is a reference document.

51–52

Differentiate between TableGen classes and C++ classes, please.

64

What kind of output files? This is way too vague.

113

Reformat to 80 columns.

265–267

This doesn't describe why "bits" fields are useful, how they are stored, or generally what their purpose is in code.

275

Why are DAGs important to TableGen?

277

Yes, and please don't submit this rev without adding some.

289–290

Not needed as part of this reference.

534

Example, not "interlude."

590

Redundant. What, specifically, does it mean to "define" a record?

630–634

"Interlude"? Unnecessarily pretentious. This is an example.

749–751

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.

839

Example, not an interlude. Also, the example should go here, not after defm.

845–846

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.

856–857

The records are not defined in the multiclasses. They are defined by the defm statement itself.

888–889

Use the term example, please. There is no other place in the LLVM documentation that mentions "interludes."

896–913

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.

950–951

Defm doesn't allow you to "gain multiple levels." Reword to be more technically accurate please.

1146

Unnecessary -- your other examples merely follow without being introduced.

1212–1213

This section looks new. Useful info.

1220

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.

1284–1285

This section looks new too. Good job.

1563

Not really relevant. All generated instruction records tend to look more or less this complex on all platforms.

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.

I will attempt to be more specific about the purpose of TableGen. I don't think I can list "all" the uses of it, however, since it is a general-purpose tool.

I will address your other points and resubmit in the next few days. Thanks for the critique.

I will attempt to be more specific about the purpose of TableGen. I don't think I can list "all" the uses of it, however, since it is a general-purpose tool.

The uses of TableGen are finite and enumerable. An effort has been made to list all the uses of TableGen -- see llvm/docs/TableGen/BackEnds.rst . I am not convinced these descriptions are canonical.

I will attempt to be more specific about the purpose of TableGen. I don't think I can list "all" the uses of it, however, since it is a general-purpose tool.

The uses of TableGen are finite and enumerable. An effort has been made to list all the uses of TableGen -- see llvm/docs/TableGen/BackEnds.rst . I am not convinced these descriptions are canonical.

The uses are finite within the LLVM and Clang projects. But nothing stops someone outside these projects from using it.

https://www.embecosm.com/2015/04/14/utilizing-tablegen-for-non-compiling-processes/

The uses are finite within the LLVM and Clang projects. But nothing stops someone outside these projects from using it.
https://www.embecosm.com/2015/04/14/utilizing-tablegen-for-non-compiling-processes/

That project does not seem to be part of LLVM, so it's not clear to me why the reference guide should document it. Anyway, as you prefer.

The uses are finite within the LLVM and Clang projects. But nothing stops someone outside these projects from using it.
https://www.embecosm.com/2015/04/14/utilizing-tablegen-for-non-compiling-processes/

That project does not seem to be part of LLVM, so it's not clear to me why the reference guide should document it. Anyway, as you prefer.

Oh, I don't think that the reference should document it. I was just pointing that an exhaustive list of TableGen uses is not possible.

Paul-C-Anagnostopoulos marked 13 inline comments as done.Aug 16 2020, 10:04 AM

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.

John: that is what you said about the heading for the let statement. I'm not sure I understand your objection. Can you elaborate?

Paul-C-Anagnostopoulos marked 11 inline comments as done.Aug 16 2020, 10:35 AM

John: that is what you said about the heading for the let statement. I'm not sure I understand your objection. Can you elaborate?

After reviewing the other headings, I see that my comment is in error. Please mark the comment as done.

Paul-C-Anagnostopoulos marked an inline comment as done.

I have incorporated John's comments. If this is now acceptable, I would appreciate someone committing it for me.

First of all, thank you for doing this. I haven't gone through it fully yet. The main thing that is missing in my opinion is to remove stuff from the existing files. You include here a listing of TableGen syntax -- that's fine, but it's now redundant with what's in TableGen/LangRef.rst, and this redundancy is a very bad place for us to be in.

llvm/docs/TableGen/ProgRef.rst
58

This is only partially true. Anonymous defs are a thing. They end up having a standing name like anonymous_NNNN, but that doesn't tend to be very useful.

nhaehnle added inline comments.Aug 17 2020, 11:24 AM
llvm/docs/TableGen/ProgRef.rst
308–309

... and an optional value.

603

Missing a colon?

First of all, thank you for doing this. I haven't gone through it fully yet. The main thing that is missing in my opinion is to remove stuff from the existing files. You include here a listing of TableGen syntax -- that's fine, but it's now redundant with what's in TableGen/LangRef.rst, and this redundancy is a very bad place for us to be in.

You are most welcome. My plan was to get this committed first. Note that this includes an update to the TableGen Overview to refer to this document rather than the two old ones. A search turns up no other references to the two old documents in the Clang or LLVM documentation. Then once this is committed, we can delete the two old files. (Is that what we do, delete them?)

Shall I wait for additional review comments from you?

First of all, thank you for doing this. I haven't gone through it fully yet. The main thing that is missing in my opinion is to remove stuff from the existing files. You include here a listing of TableGen syntax -- that's fine, but it's now redundant with what's in TableGen/LangRef.rst, and this redundancy is a very bad place for us to be in.

You are most welcome. My plan was to get this committed first. Note that this includes an update to the TableGen Overview to refer to this document rather than the two old ones. A search turns up no other references to the two old documents in the Clang or LLVM documentation. Then once this is committed, we can delete the two old files. (Is that what we do, delete them?)

Why not just delete them at the same time? (them = LangIntro.rst and LangRef.rst, I assume) Making the change atomically like that feels more "right" to me. And after actually scanning those two docs again, yes, I agree that we can just delete them.

Shall I wait for additional review comments from you?

I've sent a few more a few minutes ago, but that should cover it as far as I'm concerned.

First of all, thank you for doing this. I haven't gone through it fully yet. The main thing that is missing in my opinion is to remove stuff from the existing files. You include here a listing of TableGen syntax -- that's fine, but it's now redundant with what's in TableGen/LangRef.rst, and this redundancy is a very bad place for us to be in.

You are most welcome. My plan was to get this committed first. Note that this includes an update to the TableGen Overview to refer to this document rather than the two old ones. A search turns up no other references to the two old documents in the Clang or LLVM documentation. Then once this is committed, we can delete the two old files. (Is that what we do, delete them?)

Why not just delete them at the same time? (them = LangIntro.rst and LangRef.rst, I assume) Making the change atomically like that feels more "right" to me. And after actually scanning those two docs again, yes, I agree that we can just delete them.

Shall I wait for additional review comments from you?

I've sent a few more a few minutes ago, but that should cover it as far as I'm concerned.

I will address your comments and submit a final patch.

@nhaehnle, but please tell me how to delete the two old files. Do I simply do a 'git rm' and then commit that along with the final ProfRef.rst?

Paul-C-Anagnostopoulos marked 3 inline comments as done.

I incorporated Nicolai's final suggestions. I also deleted the old Tablegen language intro and reference, since they are replaced by this document.

@nhaehnle: Could you commit this for me?

This revision was automatically updated to reflect the committed changes.
foad added inline comments.
llvm/docs/TableGen/ProgRef.rst
1153

Is this true? This assertion did not appear in the previous documentation, and it doesn't seem to be borne out by the grammar productions: BodyItem only includes let, defvar and assert statements.

@Joe_Nash for awareness.

kosarev added inline comments.
llvm/docs/TableGen/ProgRef.rst
197

How do we know that? There are some mentions of deprecated features in the code, but field doesn't seem to be amongst them. Also, the instruction decoder backend looks at the field mark to distinct encoding field definitions from ordinary TableGen class fields which is something we seem to rely on in our instruction definition .td's. If field is deprecated, then what's supposed to be a replacement for this use case?

Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 4:38 AM
llvm/docs/TableGen/ProgRef.rst
197

We were hoping that no one would need to use it in the future, which is why it is flagged as deprecated. At some point I am supposed to try to figure out what to do about the current uses.

603

The colon is part of the RecordBody production.

1153

It is not true. Feel free to get rid of that sentence. Good catch.

foad added inline comments.May 23 2022, 7:20 AM
llvm/docs/TableGen/ProgRef.rst
1153
kosarev added inline comments.May 23 2022, 7:38 AM
llvm/docs/TableGen/ProgRef.rst
197

So if for the current uses there is no alternative and as of today little we can do to avoid using field there, then what is the purpose of officially declaring it deprecated? Wouldn't it save us some concerns and confusion to postpone such a declaration until we have a better solution?

llvm/docs/TableGen/ProgRef.rst
197

I was hoping to prevent new uses. If you want to reword the statement to say that rather than using the term "deprecated," go for it.

kosarev added inline comments.
llvm/docs/TableGen/ProgRef.rst
197

I see, thanks. Followed up in https://reviews.llvm.org/D126290.