Change RSchedule to use maps instead of lists for lookups.

[nanavati]

Bad performance cases for this may not be common, but I have encountered one.

[quark17]

Cool, sounds reasonable. Code looks fine, but I have one question.

The expected Verilog changed in one example, and a signal name changed from MUX_accumulator$write_1__SEL_1 to MUX_multiplier$write_1__SEL_1. These are both submodules (accumulator and multiplier) and I would guess that the conditions under which they are being written are the same, so CSE has unified the mux selectors for both into one definition, under one name. I would guess that the choice of name depends on the order they appear in a list, and that order has changed.

All good, but I worry that the order may not be stable, and perhaps we care about that? There are two places where the map is flattened: one is for printing, where sortRatForPrint is applied, and the other is in AState, where no sorting is performed. It's probably inefficient to sort the entire output of M.toList there, but I wonder if we could one of the following: sort the list somewhere later (for example, sort inside mkBlob that is being immediately mapped to the list) or preserve information as map for longer (and sort when it ultimately stops being a map). (I haven't taken time to remind myself what mkBlob does, to know if there's an obvious answer here.)

[nanavati]

I tried just doing the sort in AState too and it didn't show up in my profile so I thought that was the easiest and cleanest fix.

[quark17]

I was about to merge this, but noticed one last question.

[quark17]

This silently replaces an existing key. In keeping with your use of internal errors elsewhere, did you want to use an insert function reports an internal error when the key exists?

[nanavati]

Looking at the code more carefully, those keys come from the vertices of a GraphMap, which uses a map internally, so they can't conflict. That makes me think an internalError is redundant and not really worth it. Maybe it is worth a comment, but I don't feel strongly about that either way.

In fact, looking at rSchedule, the keys for the final map come from an M.toList rMap so they can't conflict either. Maybe we should take internalError out? Though, again, I don't feel strongly about it either way.

The case I do feel strongly enough about is the error in AAddSchedAssumps. I think we should keep that one because modifying an APackage like that dicey enough that we need every extra check.

What do you think?

[quark17]

I don't feel strongly. I can go with whatever you want.

A comment wouldn't hurt. I assume there's no performance impact to using a function with an error.

[nanavati]

I ended up taking out the internalError for rSchedule and switching to a comment because I thought the code looked nicer and closer to what was there before.

[quark17]

I made some tiny cleanups. If you have no objection, I'll squash merge it. Thank you!

[nanavati]

Good catches! Merge away.