Skip to content

PeepholeOpt: Stop allocating tiny helper classes (NFC) #123936

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
Merged
merged 1 commit into from
Jan 23, 2025
Merged

PeepholeOpt: Stop allocating tiny helper classes (NFC) #123936

merged 1 commit into from
Jan 23, 2025

Conversation

arsenm
Copy link
Contributor

@arsenm arsenm commented Jan 22, 2025

This was allocating tiny helper classes for every instruction
visited. We can just dispatch over the cases in the visitor
function instead.

@arsenm arsenm mentioned this pull request Jan 22, 2025
Merged
@arsenm Graphite App
Copy link
Contributor Author

arsenm commented Jan 22, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@arsenm arsenm mentioned this pull request Jan 22, 2025
Merged
@arsenm arsenm marked this pull request as ready for review January 22, 2025 12:28
@arsenm arsenm mentioned this pull request Jan 22, 2025
Merged
qcolombet
qcolombet approved these changes Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@qcolombet qcolombet left a comment

Choose a reason for hiding this comment

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

I only skimmed through the changes but they make sense.
Do you see any compile time improvements over these?

Otherwise LGTM.
(The PR/commit could have the NFC tag, right?)

@arsenm arsenm changed the title PeepholeOpt: Stop allocating tiny helper classes PeepholeOpt: Stop allocating tiny helper classes (NFC) Jan 23, 2025
@arsenm
Copy link
Contributor Author

arsenm commented Jan 23, 2025

Do you see any compile time improvements over these?

Not sure, I was going to check the compile time tracker later

@arsenm Graphite App
Copy link
Contributor Author

arsenm commented Jan 23, 2025 •
edited
Loading

Merge activity

  • Jan 22, 8:36 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 22, 8:57 PM EST: Graphite rebased this pull request as part of a merge.
  • Jan 22, 9:00 PM EST: A user merged this pull request with Graphite.

@arsenm arsenm force-pushed the users/arsenm/peephole-opt-remove-null-tri-check branch 3 times, most recently from 32796a9 to b761471 Compare January 23, 2025 01:54
Base automatically changed from users/arsenm/peephole-opt-remove-null-tri-check to main January 23, 2025 01:57
@arsenm
PeepholeOpt: Stop allocating tiny helper classes
c4c0592 
This was allocating tiny helper classes for every instruction
visited. We can just dispatch over the cases in the visitor
function instead.
@arsenm arsenm force-pushed the users/arsenm/peephole-opt-avoid-allocating-small-helper-structs branch from 86973bb to c4c0592 Compare January 23, 2025 01:57
@arsenm arsenm merged commit 2646e2d into main Jan 23, 2025
4 of 6 checks passed
@arsenm arsenm deleted the users/arsenm/peephole-opt-avoid-allocating-small-helper-structs branch January 23, 2025 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qcolombet qcolombet qcolombet approved these changes

@jayfoad jayfoad Awaiting requested review from jayfoad

@kparzysz kparzysz Awaiting requested review from kparzysz

@MatzeB MatzeB Awaiting requested review from MatzeB

Assignees
No one assigned
Labels
llvm:codegen
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arsenm @qcolombet