Skip to content
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

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

mikeurbach
Copy link
Contributor

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.

Copy link
Contributor

@prithayan prithayan left a 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 ?

@mikeurbach
Copy link
Contributor Author

Nope, I don't think I don't think a local target is ambiguous. If a user made a path like Foo>bar:Bar, then they are referring to the instance bar within Foo. If Foo is instantiated multiple times, that's ok, this path would still refer to the instance bar within any of those instances of Foo, and lower classes is (now) happy to emit a local hw.hierpath in this case (i.e., the hierpath would be hw.hierpath [@Foo::@bar] with a single inner name ref).

The difference in this change is if Foo happened to dedupe with something else, this makeAnnotationsAbsolute logic would have duplicated the local tracker annotation once for each instance of Foo. By the time this hits lower classes, lower classes has to error out, because there is one path, which originally referred to the instance of bar within the module Foo, and now refers to the instance of bar with each instance of Foo. Effectively, the path was expanded from one reference into many, which we will fail on. So I think this change is fine from that perspective, if a single local path came in, this makes sure a single local path comes out.

@youngar
Copy link
Member

youngar commented Oct 15, 2024

How do we end up with duplicated local trackers, anyways? Maybe the inliner could do it?

@mikeurbach
Copy link
Contributor Author

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.

@mikeurbach
Copy link
Contributor Author

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.

@mikeurbach
Copy link
Contributor Author

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.

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.
@mikeurbach
Copy link
Contributor Author

As mentioned earlier, this can only roll out once we've deprecated and stopped relying on PrefixModules. In order to support the new feature in this patch independently of the reliance on PrefixModules, I've gated this behind a flag, which defaults to the new behavior. That way, we could roll this out, and opt-in to the original behavior downstream until we are ready, or flip back at some point if necessary, etc.

@mikeurbach mikeurbach merged commit a6cf24c into main Nov 12, 2024
4 checks passed
@mikeurbach mikeurbach deleted the mikeurbach/dedup-local-paths branch November 12, 2024 17:19
Copy link
Member

@seldridge seldridge left a 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?

@mikeurbach
Copy link
Contributor Author

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?

Has there been thought on what the replacement could be?

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 "
for now". Now that we are trying to do new things we couldn't do before, perhaps it's time to update the implementation.


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:

if (!inserted) {
assert(pathInfo.loc.has_value() && "all PathInfo should have a Location");
auto diag = emitError(pathInfo.loc.value(),
"path identifier already found, paths must resolve "
"to a unique target");
diag.attachNote(entry.op->getLoc()) << "other path identifier here";
return failure();
}

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:

  • Now that we aren't doing PrefixModules anymore, do we even need this change?
  • Could we handle this in LowerClasses instead?

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.

@seldridge
Copy link
Member

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).

@mikeurbach
Copy link
Contributor Author

Maybe we bite the bullet and do the return type change in preparation for this. 😅

Maybe, at least something to consider for sure.

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants