Skip to content

[compiler-rt] Remove reference to Type::getInt8PtrTy; NFC #71570

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
Nov 14, 2023
Merged

[compiler-rt] Remove reference to Type::getInt8PtrTy; NFC #71570

merged 2 commits into from
Nov 14, 2023

Conversation

pmatos
Copy link
Contributor

@pmatos pmatos commented Nov 7, 2023

Removed as part of 7b9d73c .

@pmatos pmatos self-assigned this Nov 7, 2023
@pmatos pmatos requested review from petrhosek and david-xl November 7, 2023 19:48
david-xl
david-xl reviewed Nov 9, 2023
View reviewed changes
Copy link
Contributor

@david-xl david-xl left a comment

Choose a reason for hiding this comment

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

This file has a copy in /llvm/include/llvm/ProfileData/ that needs to be in sync with this one.

@pmatos
Copy link
Contributor Author

pmatos commented Nov 10, 2023

This file has a copy in /llvm/include/llvm/ProfileData/ that needs to be in sync with this one.

Thanks. I will fix that.

@pmatos pmatos requested a review from david-xl November 10, 2023 09:51
@petrhosek
Copy link
Member

This file has a copy in /llvm/include/llvm/ProfileData/ that needs to be in sync with this one.

Thanks. I will fix that.

I don't see the LLVM copy updated in the latest commit, is that intentional?

@pmatos
Copy link
Contributor Author

pmatos commented Nov 13, 2023 •
edited
Loading

This file has a copy in /llvm/include/llvm/ProfileData/ that needs to be in sync with this one.

Thanks. I will fix that.

I don't see the LLVM copy updated in the latest commit, is that intentional?

I copied the LLVM version into compiler-rt. Did I understand @david-xl incorrectly? The LLVM change already landed.

@petrhosek
Copy link
Member

This file has a copy in /llvm/include/llvm/ProfileData/ that needs to be in sync with this one.

Thanks. I will fix that.

I don't see the LLVM copy updated in the latest commit, is that intentional?

I copied the LLVM version into compiler-rt. Did I understand @david-xl incorrectly? The LLVM change already landed.

I see, I haven't realized that the LLVM side already landed; these should always be modified together to avoid potential ABI breakage between the compiler and the runtime.

@pmatos
Copy link
Contributor Author

pmatos commented Nov 14, 2023

This file has a copy in /llvm/include/llvm/ProfileData/ that needs to be in sync with this one.

Thanks. I will fix that.

I don't see the LLVM copy updated in the latest commit, is that intentional?

I copied the LLVM version into compiler-rt. Did I understand @david-xl incorrectly? The LLVM change already landed.

I see, I haven't realized that the LLVM side already landed; these should always be modified together to avoid potential ABI breakage between the compiler and the runtime.

My apologies, I didn't know about this file until I was told I had missed some in my LLVM commit.

@pmatos pmatos merged commit 5ef9ba7 into llvm:main Nov 14, 2023
@pmatos pmatos deleted the crtfix branch November 14, 2023 06:57
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
@pmatos @zahiraam
[compiler-rt] Remove reference to Type::getInt8PtrTy; NFC (llvm#71570)
8783f5c 
Removed as part of 7b9d73c .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrhosek petrhosek petrhosek approved these changes

@david-xl david-xl Awaiting requested review from david-xl

Assignees

@pmatos pmatos

Labels
compiler-rt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pmatos @petrhosek @david-xl @llvmbot