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

Changes to the Cluster API used by the GPU Project #3714

Merged
merged 11 commits into from
Feb 9, 2025

Conversation

richardyrh
Copy link
Contributor

Related issue:

Type of change: other enhancement

Impact: API modification

Development Phase: implementation

Release Notes

title

@jerryz123
Copy link
Contributor

Can you summarize the changes in the PR comment?

@richardyrh
Copy link
Contributor Author

@hansungk can summarize :)

(CCBUS(clusterId), ccbus)) ++ (if (coherence.nBanks == 0) Nil else List(
(CMBUS(clusterId), csbus),
(CCOH (clusterId), CoherenceManagerWrapperParams(csbus.blockBytes, csbus.beatBytes, coherence.nBanks, CCOH(clusterId).name)(coherence.coherenceManager)))),
connections = if (coherence.nBanks == 0) Nil else List(
(CSBUS(clusterId), CCOH (clusterId), TLBusWrapperConnection(driveClockFromMaster = Some(true), nodeBinding = BIND_STAR)()),
// NOTE(hansung): not sure this is necessary
(CLBUS(clusterId), CCOH (clusterId), TLBusWrapperConnection(driveClockFromMaster = Some(true), nodeBinding = BIND_STAR)()),
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between the CLBUS and the CSBUS?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want the CLBUS to be totally private, you probably don't want it to the CCOH (ClusterCoherence) device, since that exposes cluster-external memory

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks for the heads up

case class ClusterParams(
val clusterId: Int,
val clockSinkParams: ClockSinkParameters = ClockSinkParameters()
) extends HierarchicalElementParams {
) extends InstantiableClusterParams[Cluster] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are many of these changes intended to let you extend Cluster?

Copy link
Contributor

@jerryz123 jerryz123 left a comment

Choose a reason for hiding this comment

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

Overall looks fine, I'm guessing this lets you extend Cluster.

Just curious what the purpose of CLBUS is

@jerryz123 jerryz123 changed the base branch from master to dev February 4, 2025 23:17
@hansungk
Copy link
Contributor

hansungk commented Feb 4, 2025

Yep, mostly to have Cluster extensible to other concrete impls.

CLBUS is intended for cluster-local interconnect that's isolated from the outside system bus, which I think CSBUS connects to. Used for the SIMT core <-> scratchpad traffic that should not escape the cluster scope.

@jerryz123
Copy link
Contributor

Ah, can you leave CLBUS outside the scope of this PR then, and leave it in your extension to Cluster?

I think you can pass a custom ClusterBusTopologyParams

@hansungk
Copy link
Contributor

hansungk commented Feb 4, 2025

Yeah can do that, was unsure this would be a common use case, actually

@richardyrh
Copy link
Contributor Author

CLBUS changes should be moved here ucb-bar@d20e98d

@richardyrh richardyrh requested a review from jerryz123 February 9, 2025 10:06
@jerryz123 jerryz123 merged commit 47d6527 into chipsalliance:dev Feb 9, 2025
28 checks passed
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.

3 participants