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

Add Instanitate.definition to get Definition from cache. #4020

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

sequencer
Copy link
Member

@sequencer sequencer commented Apr 21, 2024

Dep on #4018

This issue is designed for the use case of OM, but also apply to the future linking flow.

The type of Class should always be deduped at chisel elaboration time, since for each elaboration, newly generated Class has a different type to previous one.
Thus we need to force user to pass Definition from top to down though hierarchy. I think is a bad user experience. :(
Generally, all OM Class are cacheable since it only containing simple value or no values. This makes cache always work.
Thus user only need to use Instanitate.definition(new SomeOM) to elaborate SomeOM or fetch SomeOM from builder cache if possible.

Contributor Checklist

  • 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?
  • Did you add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • Feature (or new API)

Desired Merge Strategy

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

Release Notes

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@sequencer sequencer added the Feature New feature, will be included in release notes label Apr 21, 2024
@sequencer sequencer marked this pull request as draft April 21, 2024 02:30
@sequencer sequencer force-pushed the defination-cacheable branch 2 times, most recently from cb097f1 to 88a4d81 Compare April 21, 2024 12:21
@sequencer sequencer changed the title Move cache logic from Instanitate to Defination Add Instanitate.definition to get Definition from cache. Apr 21, 2024
@sequencer sequencer marked this pull request as ready for review April 21, 2024 14:33
}

private object internal {
// impl cannot be private, but it can be inside of a private object which hides it from the public

def instance[A <: BaseModule: c.WeakTypeTag](c: Context)(con: c.Tree): c.Tree = {
Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea on how to dedup these macro, since it transforms args... seek help from @jackkoenig

Copy link
Member

@SpriteOvO SpriteOvO left a comment

Choose a reason for hiding this comment

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

LGTM. From a user perspective, this indeed provides a better convenience.

@sequencer sequencer added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Apr 24, 2024
Copy link

linux-foundation-easycla bot commented Apr 24, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: sequencer / name: Jiuyang Liu (81d4cf7)

@chiselbot chiselbot merged commit 08edb71 into main Apr 24, 2024
17 checks passed
@chiselbot chiselbot deleted the defination-cacheable branch April 24, 2024 23:16
): Instance[A] = Instance.do_apply(_defination(args, f))(sourceInfo)

/** This is not part of the public API, do not call directly! */
def _defination[K, A <: BaseModule: ru.WeakTypeTag](
Copy link
Contributor

Choose a reason for hiding this comment

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

Slight typo, should be _definition, here and below.

Copy link
Member Author

Choose a reason for hiding this comment

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

my bad! Super sorry!

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, we just should fix before 7.0.0-M2/RC1!

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I'll fix that after getting up tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants