[FIRRTL] Remove convention verification for InstanceChoice#10066
[FIRRTL] Remove convention verification for InstanceChoice#10066
Conversation
uenoku
changed the title
| std::vector<FModuleLike> ops; | ||
| auto &instanceGraph = getAnalysis<InstanceGraph>(); | ||
| // Symbol Table | ||
| auto &symTbl = getAnalysis<SymbolTable>(); |
There was a problem hiding this comment.
considered to replace symTbl with instanceGraph but it was challenging due to parallelization and instance op mutation.
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
General approach looks good. Could the name be changed to be more explicit about what this is actually checking?
I have a longer suggestion about integrating this with InstanceInfo. However, that is not blocking and can be handled in a follow-up as appropriate.
| /// Return true if the module instance graph node requires scalarization. | ||
| bool isScalarizeRequired(InstanceGraphNode *node); |
There was a problem hiding this comment.
This is really a "is this instantiated by instance choice" which then means, only elsewhere, this should be scalarized.
At minimum, this would be better if renamed.
Going further, this feels like something that would be better handled in the InstanceInfo analysis which it could then just return. This would also be more efficient for repeated calls (though I don't think we do have repeated calls).
There was a problem hiding this comment.
Fair point. In that case I think it's better not to expose it to a header so I simply inlined the logic with comment.
fabianschuiki
left a comment
fabianschuiki
left a comment
There was a problem hiding this comment.
LGTM! I'm not very familiar with the convention mechanism, but allowing certain modules to enforce a scalarized convention feels reasonable.
3 participants
Uh oh!
There was an error while loading. Please reload this page.