Builder: optional names for cells

[anshumanmohan]

In #1635 we proposed having the builder optionally generate its own names for cells instead of asking the user for names.

I have sketched this out for just one cell (eq). Currently, only two eDSL files (fifo and queue_call) actually create eq-cells without passing names for those cells. The other Python files have been updated to accommodate the new order of arguments and so you'll see a diff, but they are still passing the optional name argument and are therefore unexciting.

Please take a look and tell me what you think. This is the kind of change that will create a lot of busy work and a large diff, so I wanted to get a green light on my plan before going too far.

The obvious con of this approach is that the smallest cell- and group-names will lose semantic meaning, and the Calyx code will become harder to read. The "chunkier" pieces, e.g. control logic, will still have names as they do now. The counterargument is something like "Good, Calyx code is not for reading. If the eDSL code becomes easier to write because of this, that's a win."

After further discussion and similar changes in other cells, this will close #1635.

[rachitnigam]

I'm generally in favor of this approach: names are hints and most compiler IRs don't attempt to guarantee that they will be preserved. Exposing this API serves the same purpose: name hints are nice for generating debuggable code but you don't have to do that at the very start of building your compiler frontend.

[anshumanmohan]

Thanks for taking a look, Rachit. I have since gone through and made names for all the other basic cells optional as well. I have piped the changes through the eDSL code. This is now ready for a quick additional review, then a merge.

[rachitnigam]

Looks like some of the changes have broken the systolic array script.

[anshumanmohan]

Aye, it's because the order of arguments had to be changed (to allow the names of cells to be optional). The fix is simple, I just need to sleuth around and patch it in various places. The eDSL has more users than I realized!

[rachitnigam]

Yup! It's used in production 😆! It might be useful to put it asserts for things like names not being numbers etc. in case that helps catch some of these problems. In general, I'm a big believer in use of asserts, especially in python because the absence of types makes me feel naked.

[anshumanmohan]

Whoo, fixed! Now really ready for a re-review.

I will flag that cells are currently given names using a janky system involving random numbers. For instance, to name an add cell:

name = name or f"add_{random.randint(0, 2**32)}"

I'd appreciate any thoughts about a system that is clean and is guaranteed to avoid clashes!

[rachitnigam]

The normal approach to hygiene is having some sort of name generator that gets to see all defined names in scopes where conflicts might occur. For example, the Calyx Rust ir::Builder defines one for each component. The random name generation might not be a great idea for determinacy which is useful for the kind of snapshot testing we do.

[anshumanmohan]

Take a look now? I have a little loop that tries new names until it finds a fresh one. I still use random, but I seed it at the moment of ComponentBuilder creation, so that should stabilize it for snapshot testing. We have done this for Pollen in the past for similar reasons.

[sampsyo]

Nice!!!! I really like the idea of making names optional throughout. Thanks for doing the heavy lifting to track down the users and fix the argument order! It might be a really useful thing at this point to add a MyPy setup and to run it in CI as much as is feasible to help catch stuff like this during refactoring…

I left a suggestion for how to avoid the last remnants of randomness, and a few random code-level suggestions.