Skip to content

[PIPELINER] Use AttrHelper (mostly NFC) #6437

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

Merged
merged 3 commits into from
Apr 16, 2025
Merged

Conversation

sjw36
Copy link
Contributor

@sjw36 sjw36 commented Apr 9, 2025

  • Converted ad-hoc attribute handling to AttrHelpers to ensure consistency
  • Changed tt_latency to tt.latency in AssignLatencies?

sjw36 added 2 commits April 9, 2025 10:19
* Converted ad-hoc attribute handling to AttrHelpers to ensure consistency
* Changed `tt_latency` to `tt.latency` in AssignLatencies
@sjw36 sjw36 requested a review from ptillet as a code owner April 9, 2025 15:56
@sjw36
Copy link
Contributor Author

sjw36 commented Apr 9, 2025

@pawelszczerbuk is this change correct? I couldn't see where tt_latency was set on any ops.

Copy link
Collaborator

@Mogball Mogball left a comment

Choose a reason for hiding this comment

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

This seems like a good thing. Can we delete kLatencyAttrName now?

}
}

DenseMap<Operation *, int> mlir::triton::deserializeLatencies(Operation *op) {
auto helper = op->getContext()
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: can you add a shorthand for this in Triton/Dialect.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! added.

Copy link
Contributor Author

@sjw36 sjw36 Apr 15, 2025

Choose a reason for hiding this comment

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

posted these upstream here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well.. I guess Medhi and River are not big fans..

Copy link
Collaborator

Choose a reason for hiding this comment

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

They tend to be picky :P

@sjw36
Copy link
Contributor Author

sjw36 commented Apr 15, 2025

This seems like a good thing. Can we delete kLatencyAttrName now?

Not sure as there is still some ambiguity with tt.latency and tt_latency.. ping @pawelszczerbuk

@Jokeren Jokeren merged commit 3a91f81 into triton-lang:main Apr 16, 2025
7 of 8 checks passed
njriasan pushed a commit to njriasan/triton that referenced this pull request Apr 18, 2025
* Converted ad-hoc attribute handling to AttrHelpers to ensure
consistency
* Changed `tt_latency` to `tt.latency` in AssignLatencies?
FindHao pushed a commit to FindHao/triton that referenced this pull request Apr 30, 2025
* Converted ad-hoc attribute handling to AttrHelpers to ensure
consistency
* Changed `tt_latency` to `tt.latency` in AssignLatencies?
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