-
Notifications
You must be signed in to change notification settings - Fork 299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIRRTL] Don't force non-local trackers in Dedup. #7709
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but do we need to handle ambiguous paths in LowerClass because of this ?
Nope, I don't think I don't think a local target is ambiguous. If a user made a path like The difference in this change is if Foo happened to dedupe with something else, this |
How do we end up with duplicated local trackers, anyways? Maybe the inliner could do it? |
Likely inliner, potentially others. I need to chase this down and resolve it. I added dedupe of local trackers here in Dedup similar to how we do it for the local dont touch annotations, but in some testing downstream I'm seeing other sources of duplicated local trackers that I need to chase down. |
I think the last commit is still needed here. But the problem I was seeing downstream was actually an interaction with PrefixModules, which can also lead to duplicate annotations in cases like the examples here. That is more fundamental; I don't think there is anything we can do there with the current representation of trackers as annotations, so we'll need to hold this until module prefixes are applied by Chisel and not annotations. I do think we'll want this change at some point. |
And, now that I think about this, I may have been mistaken; the last commit here was probably not needed, and was me confusing the interaction with PrefixModules with the logic in Dedup. |
16cfd9b
to
6cf1dd0
Compare
The previous behavior for local trackers would be to clone the tracker into duplicate copies for each unique instance. This will fail in LowerClasses, because one path op that used to refer to a single entity as a local reference would now refer to every hierarchical instance of that entity. One path can't refer to multiple things, so if the user specified a local reference, allow that to pass through without expanding it into every hierarchical instance.
6cf1dd0
to
05c592a
Compare
As mentioned earlier, this can only roll out once we've deprecated and stopped relying on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, only seeing this now.
One nit, one broad concern.
Nit: this option disable-local-annotations
is inappropriately named and described. This only affects OM tracker annotations and nothing else, correct? Can this be made more obvious that this is only affecting the way that tracker annotations dedup and not all annotations as it seems to imply?
I've pushed back very strongly on this kind of thing before, primarily because it is an information-destroying operation. All annotations are supposed to have the behavior of becoming non-local on deduplication (with the one exception of don't touch... which is bad). This is the information preserving thing to do. Somebody annotated that these two things, they are then made the same by a pass, the correct thing is always to preserve that difference via a path. Downstream passes then need to understand what to do with duplicate information and can apply their interpretation of what a duplicate annotation means.
Usually when this comes up it means that an annotation is being inappropriately used and a different representation is needed (attribute, intrinsic, operation, etc.). I'm fine with this as a stop-gap towards something better (attribute, intrinsic, operation, etc.). Has there been thought on what the replacement could be?
Regarding the option name--yes I can make that more explicit, since it's really scoped to OMIR tracker annotations. Regarding the broader concern... This is intended to handle cases like what I described in the earlier comments, especially the hidden comments from discussion with @youngar. This will come up more as we start to use D/I in Chisel for more things. Lots of our FIRRTL support for Path properties was built on previous assumptions, like the idea that you could always form a unique hierarchical path to the targeted entity. These assumptions won't hold when you start using definitions and instances, and one valid way to relax these assumptions is to allow local paths, rather than trying to enforce the older hierarchical path assumptions. I know you're concerned about losing information, but I think this is possible to do without loss of information--if an annotation is local, it is tracking a module or something inside a module. If that module deduplicates with another module, why do we need to preserve these two things that are now the same used to be different?
I don't know if this is written down somewhere, but what we really want is to stop using annotations for OMIR, and instead pass values through the IR more like we did with probes. This will require, among other things, the ability to "take a reference" to an instance, which may necessitate changing firrtl.instance to have its own type. This is where @rwy7 started over a year ago, but after a decent amount of yak shaving, we decided to abandon that design point and use annotations " All that said, the specific thing this was trying to deal with is the situation where have some Path using an OMIR tracker that locally refers to a module or something inside a module. The way we've implemented Paths and OMIR trackers, there is a distinct attribute linking the Path op to the local annotation. This code path could blow out that local annotation to multiple non-local annotations. So, we'll have one Path op, but its distinct attribute shows up multiple times in the IR, and we'll hit this: circt/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp Lines 719 to 726 in 097483d
Maybe the right thing to do here is not to handle this in LowerClasses... we could keep splatting out local annotations into non-local annotations, and the distinct attribute handling code in LowerClasses could check if they all refer to the same thing. So I think I want to check a couple things:
It might be the case that we don't need to relax this behavior of Dedup, and can keep the information for longer, so it is up to the passes to deal with duplicated annotations. |
This reverts commit a6cf24c.
Thanks for the detailed comments and analysis, Mike. If it wasn't obvious, I missed the entire discussion on this! Yes, I agree that the right thing is to change the instance type (or add an additional return that is a reference to the instance, or have a dedicated op that takes an inner symbol and returns a reference...). I'm also going to run into the instance return type problem with one of the solutions considered for replacing Grand Central Views which requires vector instantiation. There was always general consensus that vector instantiation in FIRRTL made sense. However, the early choice to use multiple return types makes this weird. Maybe we bite the bullet and do the return type change in preparation for this. 😅 And, I'm not generally opposed if this PR does come back post-revert (nor did I think it necessarily needed to be reverted). Per-annotation dedup behavior is what I'm generally afraid of as a solution. Also, it was unstated that I don't think don't touch should be an annotation at this point (which would remove the other special annotation handling case). |
Maybe, at least something to consider for sure.
Understood.. I eagerly reverted because I'm now questioning if this was even necessary, and I think it could be re-landed easily, either as-is in a pinch, or ideally with a different approach that doesn't require per-annotation dedup behavior. I think we should be able to do what you were getting at earlier: "Downstream passes then need to understand what to do with duplicate information and can apply their interpretation". I believe I had tried that originally, and got stuck because I was also dealing with the interaction with module prefixing as a FIRRTL transform, and not understanding what the root issue was. With that out of the way, either the problem this PR was trying to solve would go away, or it could be solved in the relevant downstream pass. |
The previous behavior for local trackers would be to clone the tracker into duplicate copies for each unique instance. This will fail in LowerClasses, because one path op that used to refer to a single entity as a local reference would now refer to every hierarchical instance of that entity. One path can't refer to multiple things, so if the user specified a local reference, allow that to pass through without expanding it into every hierarchical instance.