Skip to content

[NFC] Fix #106873 - update assignment tracking docs #106959

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 2 commits into from
Sep 2, 2024
Merged

[NFC] Fix #106873 - update assignment tracking docs #106959

merged 2 commits into from
Sep 2, 2024

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Sep 2, 2024

No description provided.

@OCHyams OCHyams requested a review from jryans September 2, 2024 09:48
@OCHyams OCHyams mentioned this pull request Sep 2, 2024
Closed
@OCHyams OCHyams requested review from jmorse and SLTozer September 2, 2024 10:36
jryans
jryans reviewed Sep 2, 2024
View reviewed changes
Copy link
Member

@jryans jryans left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for working on this!

The TODO list still says it's an "experimental work in progress", which reads a bit strangely for something that's now enabled by default. Perhaps reword that bit as well?

I assume the rest of the TODO list is still valid, since you haven't removed any items here...?

@OCHyams
Copy link
Contributor Author

OCHyams commented Sep 2, 2024

The TODO list still says it's an "experimental work in progress", which reads a bit strangely for something that's now enabled by default. Perhaps reword that bit as well?

Agreed, done

I assume the rest of the TODO list is still valid, since you haven't removed any items here...?

Unfortunately so. Actually, while we're here I've added an item.

jmorse
jmorse approved these changes Sep 2, 2024
View reviewed changes
Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

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

LGTM as a status update; IIRC we didn't turn it on everywhere due to the performance implications in LTO? Now that we've radically changed the storage for variable debug-info, perhaps that's now something we can revisit.

jryans
jryans approved these changes Sep 2, 2024
View reviewed changes
Copy link
Member

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me as well! 😄

@OCHyams OCHyams merged commit b6a4ab5 into llvm:main Sep 2, 2024
7 of 8 checks passed
@OCHyams OCHyams deleted the at-docs branch September 2, 2024 13:58
qiaojbao pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Sep 30, 2024
Merged main:b6a4ab5a12c9ced0642769e4b2d8f77859541ba8 into amd-gfx:6ab…
3cfe7da 
…9923b43c0

Local branch amd-gfx 6ab9923 Merged main:357bd61744bb8cc2b9b07447294fa977e5758550 into amd-gfx:4cb01f455ead
Remote branch main b6a4ab5 [NFC] Fix llvm#106873 - update assignment tracking docs (llvm#106959)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jryans jryans jryans approved these changes

@jmorse jmorse jmorse approved these changes

@SLTozer SLTozer Awaiting requested review from SLTozer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@OCHyams @jryans @jmorse