[InnerSymbolTable] Expose a method to add an InnerSymTarget. #9446
Conversation
This allows us to add new symbols to the InnerSymbolTable. This is useful for passes to maintain an up-to-date InnerSymbolTable throughout mutations. The implementation simply factors out the addition done in the initial walkSymbols called by InnerSymbolTable::get, and exposes an API to call that directly. This means that it is a failure to attempt to add a symbol which is already in the InnerSymbolTable. To exercise this, new unit tests were added for InnerSymbolTable.
dtzSiFive
left a comment
dtzSiFive
left a comment
There was a problem hiding this comment.
Thanks, this is nice and straightforward and helps preserve IST's!
Nit about the test maybe actually setting the inner symbol on the wire before adding it to the (updating) IST, but shrug.
|
|
||
| auto innerSymTarget = InnerSymTarget{wire1}; | ||
|
|
||
| LogicalResult result1 = innerSymbolTable.add(name, innerSymTarget); |
There was a problem hiding this comment.
Probably we want to update wire1 so that it actually has the inner symbol?
Otherwise, constructing a new inner symbol table over testOp wouldn't have the new symbol.
I could see add defensively checking that this is the case (on debug builds)?
Related: The way to get a reasonable new inner symbol name is using ISN (InnerSymbolNamespace), or utilities like those in FIRRTLUtils.h (that use InnerSymbolNamespace). It seems a shame to not have those not capable of updating the IST should that be desired.
The tentative plan was to unify IST and ISN, so maybe not worth this until then, and users manually update their IST as desired?
There was a problem hiding this comment.
Maybe I should just put effort into unifying IST and ISN... I don't need to get this in before that
| // See https://llvm.org/LICENSE.txt for license information. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// |
There was a problem hiding this comment.
Nice! Thanks for adding this! 💯
| InnerSymbolTable(InnerSymbolTable &&) = default; | ||
| InnerSymbolTable &operator=(InnerSymbolTable &&) = default; | ||
|
|
||
| // Add a fresh mapping, or fail if the symbol name already exists. |
There was a problem hiding this comment.
I feel like this isn't a thing, but should it be noted that this generates a diagnostic for the failure case?
This allows us to add new symbols to the InnerSymbolTable. This is useful for passes to maintain an up-to-date InnerSymbolTable throughout mutations.
The implementation simply factors out the addition done in the initial walkSymbols called by InnerSymbolTable::get, and exposes an API to call that directly.
This means that it is a failure to attempt to add a symbol which is already in the InnerSymbolTable.
To exercise this, new unit tests were added for InnerSymbolTable.