Skip to content

Conversation

@dianqk
Copy link
Member

@dianqk dianqk commented Dec 13, 2025

Backport 6655681.

Summary:
The OpenMP handling using an offload binary should be optional, it's
only used for extra metadata for llvm-objdump. Also the triple was
completely wrong, it didn't let anyone correctly choose between ELF
and COFF handling.

(cherry picked from commit 6655681)
@dianqk dianqk requested review from jhuber6 and sarnex December 13, 2025 11:00
@dianqk
Copy link
Member Author

dianqk commented Dec 13, 2025

cc @ZuseZ4

@dyung
Copy link
Collaborator

dyung commented Dec 14, 2025

Is this a regression from a previous release? If so, which one? This is going to be the final release from the LLVM 21.x branch, so any issues from taking this change would not be fixed until LLVM 22 releases in about a month. And considering this is the final release of 21.x, we need to be very careful what changes are included. What would be the impact of not accepting this change for 21 and waiting for 22 instead?

@jhuber6
Copy link
Contributor

jhuber6 commented Dec 14, 2025

Is this a regression from a previous release? If so, which one? This is going to be the final release from the LLVM 21.x branch, so any issues from taking this change would not be fixed until LLVM 22 releases in about a month. And considering this is the final release of 21.x, we need to be very careful what changes are included. What would be the impact of not accepting this change for 21 and waiting for 22 instead?

Honestly, it's more of a feature. It makes the utility more usable but it's not strictly required nor does it fix a bug. This, I believe, comes from the Rust team wanting to more easily use this utility. We usually don't backport features so it's up to you.

@ZuseZ4
Copy link

ZuseZ4 commented Dec 14, 2025

This, I believe, comes from the Rust team wanting to more easily use this utility.

Yes indeed, I had asked for it for our rustc GPU efforts in rust-lang/rust-project-goals#109
This PR had helped me with replacing the clang-linker-wrapper invocation, since in Cargo we don't generally want to call (additional) LLVM binaries. However, Joseph mentioned how I can work around it on the rust side. I didn't get to test it yet, but (worst case) we can vendor the fix for a few months on the Rust side till we get to update to LLVM-22, which will simplify our compilation pipeline anyway by quite a bit.

Since you seem a bit concerned about the timeline, I guess either fix on the Rust side is preferred, so we can go with that.

@dyung
Copy link
Collaborator

dyung commented Dec 15, 2025

Hi, thanks for the explanation, but I think I am going to pass on integrating this change for the 21.x release, but 22.x is just a little over a month away so hopefully your issue gets better soon with that!

@dyung dyung moved this from Needs Triage to Won't Merge in LLVM Release Status Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Won't Merge

Development

Successfully merging this pull request may close these issues.

4 participants