This is an archive of the discontinued LLVM Phabricator instance.

[LLVM][TableGen][Jupyter] Add first tutorial notebook
ClosedPublic

Authored by DavidSpickett on Oct 31 2022, 8:34 AM.

Details

Summary

This first notebook covers the basics.

  • classes
  • defs
  • basic types
  • let in various forms

Everything up to multiclass, which will be the
start of the 2nd part of the tutorial.

I'd like to keep them in logical sections as
far as possible, so they are easy to digest.

Diff Detail

Event Timeline

DavidSpickett created this revision.Oct 31 2022, 8:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 8:34 AM
DavidSpickett requested review of this revision.Oct 31 2022, 8:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 8:34 AM
DavidSpickett planned changes to this revision.Nov 29 2022, 2:42 AM

Mucking about with this I realised multiple inheritance is a thing, so I'm going to note that here too.

(all the more reason to have exploration tools isn't it)

Matt added a subscriber: Matt.Dec 5 2022, 12:36 PM
  • Added section on inheritance and multiple inheritance.
  • Demonstrated more of the error states.
  • Added headings for easier browsing.

Minor wording changes and proof reading.

  • Use default values for template arguments instead of a def inside the class.
  • General wording changes.

I'm going to lock the content there until review suggestions come in.
I keep finding new language features and it'll go on forever if I keep
adding them to this.

Eymay updated this revision to Diff 503641.Mar 8 2023, 11:16 PM
Eymay added a subscriber: Eymay.

Typo fix on file name in recommended command

Rebase and put back the patch content.

You somehow managed to update the diff itself. Normally one would
either leave a comment on the part of the diff that is incorrect,
or upload your own patch (https://llvm.org/docs/Contributing.html#how-to-submit-a-patch)
to address it (if the code has already landed).

That said I've done my fair share of strange things with
arc and Phabricator :). Thanks for the note, I will fix the command.

If you have feedback on the tutorial you can add it as comments here.
If you click on the line numbers, it'll bring up a comment box.
Just remember to scroll down and click "Submit" to actually send the comments.

Eymay added inline comments.Mar 10 2023, 10:34 AM
llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb
16

I get the reasoning behind %reset but there is a risk that a complete beginner may miss this sentence and think of it an integral part of TableGen.
If it could be greyed with a syntax highligher that could help. Or a different logic for clearing cache like a %global can be used. I found it was a little annoying seeing it in every cell block.

78

Embedding links could be better

155

first let can be enclosed as let

155

It is a little difficult to track, more descriptive names instead of X,Y,Z can maybe help the reader.

Another option is to keep the structure consistent with the previous code blocks so it can be more familiar.
For example, X can be repeated as:

def X: C {
	
    let b=5;
			
}
158

This sentence can be moved after the block as it is or can be merged with the previous sentence as a broad introduction to the code block's use case. The issue I had was transitioning from a code block's explanation to the other. It would be good to keep it a single paragraph to introduce each code block like it seems in the Programmer's Reference.

236

Is this let statement relevant, it can be confusing if not.

DavidSpickett marked 4 inline comments as done.

Address review comments.

I'll see what I can do about a configurable default for %reset or not.

llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb
16

We have conflicting use cases going on.

For a tutorial where each snippet is standalone, I agree that making %reset the default would be correct.
For a tutorial where you build up a large example by combining snippets as you go, you would want a default of %global (as you put it).
For prototyping something bit by bit, the same applies.

Most Jupyter kernels default to global cells, because they're writing to an interpreter process underneath. So I've stuck with that tradition.

We could add a magic to configure it. I think "noreset" would be a better name than "global" given how the kernel works, but you get the idea.

%cellsreset on

Then if the default is not what you want for a specific cell you can do %reset/%noreset in that one cell.
I'd argue the default for the kernel should be %reset then for these tutorials we change that to %noreset.

TLDR: Yes the line noise is annoying, I will look at adding a configurable default for it.

78

I'm taking embedding to mean linking words rather than pasting the literal link in. I've updated the links to do that.

155

I think I've improved it, you tell me :)

I made the base class not a single letter name to distinguish it from the defs. Then I changed the variable name to var so that it's distinct from the def names. Finally I rearranged it a bit so that the value of var counts up and things get more complex each time.

I'm not sure what I would rename X/Y/Z to. I thought maybe "Inner", "InnerMost" but it wouldn't really be accurate imo. Maybe you have an idea there.

158

Agreed. Moved it down to below the code.

In general I think I flipped back and forth on describing the blocks first, or after the code. I did what felt right but it may be better to pick one style and that probably is description after.

236

Good spot, this is unused. I think I left it in from an earlier draft.

DavidSpickett planned changes to this revision.EditedApr 24 2023, 4:44 AM

Plan changes because this needs updating if/when configurable reset is a thing, see https://reviews.llvm.org/D149055.

Use the config command added in the previous patch to remove a bunch of %reset lines.

DavidSpickett retitled this revision from [LLVM][TableGen] Add first tutorial notebook to [LLVM][TableGen][Jupyter] Add first tutorial notebook.
DavidSpickett marked an inline comment as done.

Small changes to text.

Removed the last part about part 2 of the tutorial. Not sure when I'll
get to that, and it would just confuse readers. Better that they go back
to the folder and look around there for more notebooks.

Update error output after named arguments were added to Tablegen.

Which I may add to this, but in a later patch to prevent more churn.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 4 2023, 3:22 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

This has had some review and realistically, the only way it will get more is if/when people use it. So I'm landing this without any accepts.