8000 [PrefixModules] Rename references from the module body by nandor · Pull Request #5259 · llvm/circt · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[PrefixModules] Rename references from the module body #5259

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 25, 2023
Merged

[PrefixModules] Rename references from the module body #5259

merged 1 commit into from
May 25, 2023

Conversation

nandor
Copy link
Contributor
@nandor nandor commented May 25, 2023

Required to unblock the build of internal cores with additional constraints.

uenoku
uenoku reviewed May 25, 2023
View reviewed changes
Copy link
Member
@uenoku uenoku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not much familiar with the pass, but the change looks good to me. Could you add a test?

@nandor nandor force-pushed the dev/nandor/prefix-modules-fix branch 2 times, most recently from 50480c6 to 5642d77 Compare May 25, 2023 09:56
@nandor nandor force-pushed the dev/nandor/prefix-modules-fix branch from 5642d77 to f746a6a Compare May 25, 2023 11:11
dtzSiFive
dtzSiFive reviewed May 25, 2023
View reviewed changes
prithayan
prithayan reviewed May 25, 2023
View reviewed changes
lib/Dialect/FIRRTL/Transforms/PrefixModules.cpp
if (moduleName == oldName) {
newTarget = module.getNameAttr();
} else {
auto target = instanceGraph->lookup(moduleName)->getModule();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If InstanceGraph caches the InstanceGraphNode::nodeMap, this could be stale, with old (non-prefixed) name to module map. I am not sure, but just a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assumption is that the module name is from a child module I guess. At least in my mind, a ref in a sv.verbatim to a parent symbol is very dodgy.

10000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what behaviour should be if we reference a parent module, but even if we want to do something, I'd like to do it in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why you can't reference a parent.

fabianschuiki
fabianschuiki approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor
@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for fixing this!

fabianschuiki
fabianschuiki approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor
@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for fixing this!

fabianschuiki
fabianschuiki approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor
@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for fixing this!

fabianschuiki
fabianschuiki approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor
@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for fixing this!

@nandor
Copy link
Contributor Author
nandor commented May 25, 2023

I'm merging this in as it gets internal cores going. Let me know if it causes problems.

@nandor nandor merged commit 775725e into main May 25, 2023
@nandor nandor deleted the dev/nandor/prefix-modules-fix branch May 25, 2023 16:07
@dtzSiFive
Copy link
Contributor

I understand this is less broken, but please see this through.

Here's a simple tweak to your test that demonstrates one of the kinds of breakages I mentioned: https://github.com/llvm/circt/compare/main...dtzSiFive:circt:misc/prefix-module-breakage?expand=1 .

Running with this PR produces the following (should be caught by verifier (?!)):

  firrtl.circuit "Prefix_RewriteInnerNameRefs" {
    firrtl.module @Prefix_RewriteInnerNameRefs() {
      %wire = firrtl.wire sym @wire : !firrtl.uint<1>
      firrtl.instance mid @Prefix_Prefix2_Mid()
      sv.verbatim "{{0}}" {symbols = [#hw.innerNameRef<@Prefix_RewriteInnerNameRefs::@wire>]}
      sv.verbatim "{{0}} {{1}}" {symbols = [#hw.innerNameRef<@Prefix_RewriteInnerNameRefs::@wire>, #hw.innerNameRef<@Prefix_Prefix3_Nested::@wire>]}
    }
    firrtl.module @Prefix_Prefix2_Mid() {
      firrtl.instance nested @Prefix_Prefix2_Prefix3_Nested()
    }
    firrtl.module @Prefix_Prefix2_Prefix3_Nested() {
      %wire = firrtl.wire sym @wire : !firrtl.uint<1>
    }
  }

Notice that in the presence of multiple prefixes being applied the inner ref is updated incorrectly (missing Prefix2 on Nested reference).

@darthscsi darthscsi mentioned this pull request May 26, 2023
Open
@darthscsi
Copy link
Contributor

Opened issue #5274 and assigned it to @nandor to complete this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prithayan prithayan prithayan left review comments

@uenoku uenoku uenoku left review comments

@dtzSiFive dtzSiFive dtzSiFive left review comments

@fabianschuiki fabianschuiki fabianschuiki approved these changes

@darthscsi darthscsi Awaiting requested review from darthscsi darthscsi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nandor @dtzSiFive @darthscsi @fabianschuiki @prithayan @uenoku
0