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.

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

Bugfix - module name collision for injecting aspect #1635

Merged
merged 5 commits into from
Oct 26, 2020
Merged

Bugfix - module name collision for injecting aspect #1635

merged 5 commits into from
Oct 26, 2020

Conversation

azidar
Copy link
Contributor

@azidar azidar commented Oct 23, 2020 •
edited
Loading

Contributor Checklist

  • [N/A] Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • [N/A] Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • bug fix

API Impact

Adds private [chisel3] APIs to enable fixing module name collision behavior.

Backend Code Generation Impact

None.

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference.)

Release Notes

Bugfix: Instantiating modules in an InjectingAspect will no longer create modules with conflicting names in the original design.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.0, 3.5.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@azidar azidar requested a review from a team as a code owner October 23, 2020 17:54
@azidar azidar requested review from ducky64 and removed request for a team October 23, 2020 17:54
@azidar azidar marked this pull request as draft October 23, 2020 18:12
@azidar azidar marked this pull request as ready for review October 23, 2020 19:19
ducky64
ducky64 requested changes Oct 24, 2020
View reviewed changes
Copy link
Contributor

@ducky64 ducky64 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 more comments would be very helpful, it's not obvious what's going on and why just by looking at code...

core/src/main/scala/chisel3/internal/Builder.scala
build(f, new DynamicContext())
}

private [chisel3] def build[T <: RawModule](f: => T, dynamicContext: DynamicContext): (Circuit, T) = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right API, to expose all of dynamicContext, as opposed to perhaps something more targeted like preinitialized / reserved names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I made the function package private, which should give us the ability to change it in the future. However, I think it actually does make sense to expose the dynamic context, because I've often hit cases with the Instance API that I've been wanting ways to add context-state to a chisel execution.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

core/src/main/scala/chisel3/Module.scala Show resolved Hide resolved
src/main/scala/chisel3/aop/injecting/InjectingAspect.scala Show resolved Hide resolved
src/main/scala/chisel3/aop/injecting/InjectingAspect.scala Show resolved Hide resolved
src/test/scala/chiselTests/aop/InjectionSpec.scala Outdated Show resolved Hide resolved
src/test/scala/chiselTests/aop/InjectionSpec.scala Show resolved Hide resolved
@jackkoenig
Copy link
Contributor

@ducky64 can you re-review?

ducky64
ducky64 approved these changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@ducky64 ducky64 left a comment

Choose a reason for hiding this comment

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

lgtm, the docs definitely helped clarify behavior!

src/test/scala/chiselTests/aop/InjectionSpec.scala Show resolved Hide resolved
@azidar azidar added this to the 3.4.x milestone Oct 26, 2020
@azidar azidar added Bugfix Fixes a bug, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. labels Oct 26, 2020
@mergify mergify bot merged commit 1b6bd89 into master Oct 26, 2020
mergify bot pushed a commit that referenced this pull request Oct 26, 2020
@azidar
Bugfix - module name collision for injecting aspect (#1635)
9b28694 
* Bugfix - module name collision for injecting aspect

* Fixed mechanism to avoid module name collisions

* Added comments for reviewer feedback

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 1b6bd89)
@mergify mergify bot mentioned this pull request Oct 26, 2020
Merged
@mergify mergify bot added the Backported This PR has been backported label Oct 26, 2020
mergify bot added a commit that referenced this pull request Oct 26, 2020
@mergify @azidar
Bugfix - module name collision for injecting aspect (bp #1635) (#1639)
54b8b4d 
* Bugfix - module name collision for injecting aspect (#1635)

* Bugfix - module name collision for injecting aspect

* Fixed mechanism to avoid module name collisions

* Added comments for reviewer feedback

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 1b6bd89)

* Update InjectingAspect.scala

Add additional function for backwards compatibility.

Co-authored-by: Adam Izraelevitz <azidar@gmail.com>
@jackkoenig jackkoenig mentioned this pull request Oct 30, 2020
Merged
@azidar azidar mentioned this pull request Nov 2, 2020
Merged
11 tasks
mergify bot added a commit that referenced this pull request Nov 2, 2020
@mergify @azidar
Bugfix - adding external modules was broken (bug introduced in #1635)…
8106f18 
… (bp #1649) (#1650)

* Bugfix - adding external modules was broken (#1649)


(cherry picked from commit d21fe71)

* Remove test because not backportable

* Remove extra space

Co-authored-by: Adam Izraelevitz <azidar@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@jackkoenig jackkoenig deleted the mname-collision-aspect branch July 7, 2021 03:44
@MAFOR-TECH MAFOR-TECH mentioned this pull request Oct 24, 2021
Closed
@sequencer sequencer mentioned this pull request Nov 8, 2021
Closed
jackkoenig pushed a commit that referenced this pull request Feb 28, 2023
@albert-magyar
Add scaladoc for LogicNode and tighten LowForm-only constraint (#1635)
20fac5c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ducky64 ducky64 ducky64 approved these changes

@jackkoenig jackkoenig Awaiting requested review from jackkoenig

Assignees
No one assigned
Labels
Backported This PR has been backported Bugfix Fixes a bug, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Milestone
3.4.x
Development

Successfully merging this pull request may close these issues.
3 participants
@azidar @jackkoenig @ducky64