Skip to content

[ClangLinkerWrapper] Reintroduce missing upstream commits that remove in-house LTO in clang-linker-wrapper #16952

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
Closed
wants to merge 4 commits into from
Closed

[ClangLinkerWrapper] Reintroduce missing upstream commits that remove in-house LTO in clang-linker-wrapper #16952

wants to merge 4 commits into from

Conversation

@asudarsa
Copy link
Contributor

@asudarsa asudarsa commented Feb 10, 2025 •
edited
Loading

A couple of upstream commits were not merged due to merge conflicts.
This PR reintroduces the following upstream commits:

  1. ccd73ee
  2. 4a96081

Also, This PR restores missing changes from upstream commit aae059e1389bebe86ceb3aea159d95ca6d0823ea](f2f1a14)

Thanks

@asudarsa
[ClangLinkerWrapper] A couple of upstream commits were not merged due…
aba8371 
… to 'merge' conflicts.

This PR reintroduces the following upstream commits:
1. ccd73ee
2. 4a96081
@asudarsa asudarsa requested a review from a team as a code owner February 10, 2025 19:00
@bader
Copy link
Contributor

bader commented Feb 10, 2025

@AlexeySachkov, please, take a look. This seems to duplicate/conflict with #16926.

@asudarsa asudarsa requested a review from a team as a code owner February 10, 2025 21:56
@asudarsa
Copy link
Contributor Author

asudarsa commented Feb 10, 2025

@AlexeySachkov, please, take a look. This seems to duplicate/conflict with #16926.

Hi @bader

I had an offline discussion with @AlexeySachkov and we decided to have a separate PR for bringing in missing upstream commits and do improvements in a separate PR.
@AlexeySachkov, may be we can close #16926 ?

Thanks

asudarsa
asudarsa commented Feb 10, 2025
View reviewed changes
clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
}

Expected<StringRef> writeOffloadFile(const OffloadFile &File) {
// TODO: Remove HasSYCLOffloadKind dependence when aligning with community code.

This comment was marked as resolved.

Sign in to view

@asudarsa
Copy link
Contributor Author

asudarsa commented Feb 10, 2025 •
edited
Loading

Some changes that overlapped with changes in #16884 have been removed from this PR.

Thanks

@asudarsa
Minor bugfix
3b1090a 
Signed-off-by: Arvind Sudarsanam <[email protected]>
@AlexeySachkov AlexeySachkov mentioned this pull request Feb 11, 2025
Closed
AlexeySachkov
AlexeySachkov approved these changes Feb 11, 2025
View reviewed changes
clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
std::scoped_lock Guard(ImageMtx);
WrappedOutput.push_back(*OutputFile);
}
if (HasNonSYCLOffloadKinds) {
Copy link
Contributor

@AlexeySachkov AlexeySachkov Feb 11, 2025

Choose a reason for hiding this comment

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

If this if is supposed to be aligned to the upstream code, then I would suggest to add an explicit comment about it - it should help pulldown coordinators when dealing with merge conflicts

Copy link
Contributor Author

@asudarsa asudarsa Feb 13, 2025

Choose a reason for hiding this comment

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

I thought about this. I would prefer to track this more closely in the future than rely on co-ordinators to deal with merge conflicts.

Thanks

@asudarsa
Copy link
Contributor Author

asudarsa commented Feb 13, 2025

@intel/llvm-gatekeepers

I think this is ready to be merged.

Thanks

ldrumm
ldrumm suggested changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@ldrumm ldrumm left a comment

Choose a reason for hiding this comment

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

Authorship is not maintained. Please redo this with the original commits you list, and then someone with permissions will need to do a proper merge

@asudarsa
Copy link
Contributor Author

asudarsa commented Feb 13, 2025

Authorship is not maintained. Please redo this with the original commits you list, and then someone with permissions will need to do a proper merge

Good point. Let me take a look. It might require a new PR though. Thanks

@asudarsa
Copy link
Contributor Author

asudarsa commented Mar 5, 2025

I opened #17323
The new PR maintains authorship.
Please take a look. Closing this now.

Thanks

@asudarsa asudarsa closed this Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@mdtoguchi mdtoguchi mdtoguchi approved these changes

+1 more reviewer

@ldrumm ldrumm ldrumm requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@asudarsa @bader @ldrumm @AlexeySachkov @mdtoguchi