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

Update charter.adoc per Greg's feedback #1

Merged
merged 3 commits into from
Jun 24, 2024
Merged

Conversation

bcstrongx
Copy link
Collaborator

Revise to clarify intent of existing extensions, and add justification for second extension

Revise to clarify intent of existing extensions, and add justification for second extension

Signed-off-by: Beeman Strong <97133824+bcstrongx@users.noreply.github.com>
charter.adoc Outdated Show resolved Hide resolved
charter.adoc Outdated
* An extension that enables precise attribution of samples based on select events (e.g., instruction/uop retirement events) to the instruction that caused the counter overflow, despite implementations where the associated sampling interrupt may skid. This will provide more directly actionable information to the user, by precisely identifying the instructions that are most often experiencing performance events.
* An extension that enables sampling of instructions and/or uops, with collection of runtime metadata for the instruction/uop, including data virtual address, selecct event occurrences, and latencies incurred. Such samples can be filtered based on instruction/uop type, events incurred, or latencies observed, allowing the user to focus on samples of interest. Further, associated sampling interrupts can be skidless, allowing the user to collect additional sample state (call-stack, register values) reliably.

Each extension will be crafted to be implementation-friendly even for high-performance, out-of-order microarchitectures, aiming to require no additional performance overhead beyond that resulting from the handling of sampling interrupts. The extensions will be compatible with the H extension, and support RISC-V security objectives.

Choose a reason for hiding this comment

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

Do we want to mention zero cost as well as zero overhead. That is, also clarify that implementing the extension should (even when disabled) should have zero additional performance cost?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I understand the cost vs overhead distinction. But I wasn't thinking that we would aim to mandate no overhead, at least when the capability is enabled. Rather, my thinking was that we craft the ISA to ensure there is a viable path to an implementation that adds no overhead (besides interrupts) when the capability is enabled. But it will probably always be cheaper to implement with some slowdown, and I didn't think we wanted to forbid that.

I agree that, when the capability is not enabled, there should be no overhead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just tweaked the language there, now says

...aiming to require no performance overhead when enabled beyond that...

Choose a reason for hiding this comment

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

LGTM, I think I missed the significance of the phrase "aiming to require" first time around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that seemed confusing in retrospect. I revised it again, have a look.

fix typo, clarify that overhead is only when enabled

Signed-off-by: Beeman Strong <97133824+bcstrongx@users.noreply.github.com>
Clarify that no performance overhead is a goal, not a requirement

Signed-off-by: Beeman Strong <97133824+bcstrongx@users.noreply.github.com>
Copy link

@snehasish snehasish left a comment

Choose a reason for hiding this comment

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

LGTM

@bcstrongx bcstrongx merged commit f9b7717 into main Jun 24, 2024
@bcstrongx bcstrongx deleted the greg-charter-feedback branch June 24, 2024 20:12
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.

2 participants