[FIRRTL] Donot rename InstanceOp #5352
Conversation
prithayan
requested review from
darthscsi,
dtzSiFive and
seldridge
as code owners
prithayan
changed the title
| @@ -115,6 +115,8 @@ static StringRef chooseName(StringRef a, StringRef b) { | |||
| /// the name passed in. | |||
| static void updateName(PatternRewriter &rewriter, Operation *op, | |||
| StringAttr name) { | |||
| // Should never rename InstanceOp | |||
| assert(!isa<InstanceOp>(op)); | |||
There was a problem hiding this comment.
This method looks like it only replaces the name if the old one is "less good" (including shorter, as in the lit test).
Anyway naming a likely-to-have-multiple-results instance op with name of a node consuming its result does seem unlikely to do what we want (maybe the length comparison should compare against the full asm result name?).
If the instance has no name (or it's useless or droppable), in some sense it may be good to keep the name of the node around for readability but not sure worth trying to handle that (sanely).
There was a problem hiding this comment.
Not sure, maybe this can just return early on instance op instead of assert, and less checking on caller-side?
If you want to be sure any misuse (user calling this on an instanceop that may expect it to do something) the assert sounds good to me, just a thought. Yay asserts! 👍
Co-authored-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Co-authored-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Co-authored-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Fix for #5351