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][Inliner] Support per-field syms, update local users. #5776

Conversation

dtzSiFive
Copy link
Contributor

No description provided.

@dtzSiFive dtzSiFive force-pushed the feature/inliner-per-field-and-update-local-inner-sym-users branch from a838e8e to 4e46319 Compare August 3, 2023 19:02
@dtzSiFive dtzSiFive added the FIRRTL Involving the `firrtl` dialect label Aug 4, 2023
@dtzSiFive
Copy link
Contributor Author

dtzSiFive commented Aug 5, 2023

cc #5598 -- TODO: link issue from commit/PR, include test. ✔️

@dtzSiFive dtzSiFive force-pushed the feature/inliner-per-field-and-update-local-inner-sym-users branch from 4e46319 to 26f5c85 Compare August 7, 2023 13:42
@prithayan
Copy link
Contributor

prithayan commented Aug 7, 2023

Shouldn't the following @nla2 annotation become a local annotation, instead of being dropped,

firrtl.circuit "CollidingSymbolsFields" {
  hw.hierpath private @nla1 [@CollidingSymbolsFields::@foo, @Foo::@bar, @Bar]
  hw.hierpath private @nla2 [@CollidingSymbolsFields::@foo, @Foo::@b_0]
  firrtl.module @Bar() attributes {annotations = [{circt.nonlocal = @nla1, class = "nla1"}]} {}
  firrtl.module @Foo(in %x : !firrtl.bundle<a: uint<1>, b: uint<1>> sym [<@b_0,1,public>,<@foo,2,public>]) attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}, {circt.nonlocal = @nla2, class = "nla2"}]} {
    %b = firrtl.wire sym [<@b,1,public>,<@bar_0,2,public>] : !firrtl.bundle<a: uint<1>, b: uint<1>>
    firrtl.instance bar sym @bar @Bar()
    %1 = firrtl.ref.rwprobe <@Foo::@b_0> : !firrtl.uint<1>
    %2 = firrtl.ref.rwprobe <@Foo::@bar_0> : !firrtl.uint<1>
    %3 = firrtl.ref.rwprobe <@Foo::@foo> : !firrtl.uint<1>
  }
  firrtl.module @CollidingSymbolsFields(in %x : !firrtl.bundle<a: uint<1>> sym [<@b_0,1,public>]) {
    firrtl.instance foo sym @foo @Foo(in x : !firrtl.bundle<a: uint<1>, b: uint<1>>)
    %collision_b = firrtl.wire sym @b : !firrtl.uint<1>
    %collision_bar = firrtl.wire sym [<@bar,1,public>,<@bar_0,2,public>] : !firrtl.bundle<a: uint<1>, b: uint<1>>
  }
}

output with this PR:

firrtl.circuit "CollidingSymbolsFields" {
    hw.hierpath private @nla1 [@CollidingSymbolsFields::@bar_1, @Bar]
    firrtl.module @Bar() attributes {annotations = [{circt.nonlocal = @nla1, class = "nla1"}]} {
    }
    firrtl.module @Foo(in %x: !firrtl.bundle<a: uint<1>, b: uint<1>> sym [<@b_0,1,public>, <@foo,2,public>]) attributes {annotations = [{circt.nonlocal = @nla2, class = "nla2"}]} {
      %b = firrtl.wire sym [<@b,1,public>, <@bar_0,2,public>] : !firrtl.bundle<a: uint<1>, b: uint<1>>
      firrtl.instance bar sym @bar @Bar()
      %0 = firrtl.ref.rwprobe <@Foo::@b_0> : !firrtl.uint<1>
      %1 = firrtl.ref.rwprobe <@Foo::@bar_0> : !firrtl.uint<1>
      %2 = firrtl.ref.rwprobe <@Foo::@foo> : !firrtl.uint<1>
    }
    firrtl.module @CollidingSymbolsFields(in %x: !firrtl.bundle<a: uint<1>> sym [<@b_0,1,public>]) {
      %foo_x = firrtl.wire sym [<@b_0_0,1,public>, <@foo_0,2,public>] : !firrtl.bundle<a: uint<1>, b: uint<1>>
      %foo_b = firrtl.wire sym [<@b_1,1,public>, <@bar_0_0,2,public>] : !firrtl.bundle<a: uint<1>, b: uint<1>>
      firrtl.instance foo_bar sym @bar_1 @Bar()
      %0 = firrtl.ref.rwprobe <@CollidingSymbolsFields::@b_0_0> : !firrtl.uint<1>
      %1 = firrtl.ref.rwprobe <@CollidingSymbolsFields::@bar_0_0> : !firrtl.uint<1>
      %2 = firrtl.ref.rwprobe <@CollidingSymbolsFields::@foo_0> : !firrtl.uint<1>
      %collision_b = firrtl.wire sym @b : !firrtl.uint<1>
      %collision_bar = firrtl.wire sym [<@bar,1,public>, <@bar_0,2,public>] : !firrtl.bundle<a: uint<1>, b: uint<1>>
    }
  }

@dtzSiFive
Copy link
Contributor Author

Shouldn't the following @nla2 annotation become a local annotation, instead of being dropped,

Thank you for pushing for breakages, this is a tricky thing to handle! Really appreciate it!

Regarding the example-- ah, yes. Okay so I somewhat intentionally didn't bother trying to make this work (note it presently crashes on your input) re:updating old-style NLA's (leaf reference isn't just the module, but points to the symbol in particular) since that looked super hard and I /think/ we don't need that anymore?

I'm not sure, though, will investigate a bit more. Please LMK if that sounds wrong! If not, I actually meant to push on (and perhaps bug you about) whether no longer needing to support leaf references (old-style NLA's) could significantly simplify the pass? The only symbols in NLA's we should need to update these days, I think/hope, are the ones on instances (which can't have per-field symbols anyway). WDYT?

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.

I think I agree, we need not complicate the implementation further to support old-style NLAs.
LGTM.

@dtzSiFive dtzSiFive merged commit 1a28790 into llvm:main Aug 8, 2023
5 checks passed
@dtzSiFive dtzSiFive deleted the feature/inliner-per-field-and-update-local-inner-sym-users branch August 8, 2023 15:13
@dtzSiFive
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FIRRTL][Inliner] Handle inner symbols with fieldID != 0
2 participants