This is an archive of the discontinued LLVM Phabricator instance.

Revert "Re-land [LLD] Remove global state in lldCommon"
AbandonedPublic

Authored by krzysz00 on Feb 8 2022, 9:32 AM.

Details

Reviewers
aganea
MaskRay
bondhugula
herhut
Group Reviewers
Restricted Project
Summary

This reverts commit 83d59e05b201760e3f364ff6316301d347cbad95.

D108850 is being reverted due to breaking multiple aspects of how the
LLVM toolchain interacts with HIP, both in MLIR and for compilation on
Windows.

Diff Detail

Event Timeline

krzysz00 created this revision.Feb 8 2022, 9:32 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
krzysz00 requested review of this revision.Feb 8 2022, 9:32 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
ormris removed a subscriber: ormris.Feb 8 2022, 9:42 AM

@aganea Can we expect fixes for both the issues with this revision (the HIP-on-Windows problems noted last night and the MLIR breakage) shortly or should we revert while y'all work out fixes?

@krzysz00 I will revert and re-land with both fixes.

All things considered I think in the short term it is better to aim for small patches/fixes that can be transplanted to release/14.x if need be. I've posted D119277 for the issue you were seeing @krzysz00, but I am still planning on proper fix over the next week or so.

MaskRay requested changes to this revision.EditedFeb 8 2022, 1:23 PM

Thanks to @aganea for working on this. I agree that lld/ELF is not ready for library usage. So far the arguments have always been "the library usage is easy since lld headers happen to be exposed", but do not justify that it should be done this way. Most use cases can be replaced with spawning a new process.

For lld/ELF, the library usage is unsupported and may happen to work for some use cases. But please move away from the library usage, or expect that it may break at any time, and don't use "it worked for some time for us" as an argument for reverts like https://reviews.llvm.org/D119257

If anything needs to be reverted, the original change adding lld::elf::link for AMDGPU usage should be reverted instead.

This revision now requires changes to proceed.Feb 8 2022, 1:23 PM

(The discussion seems to be happening in two places: https://github.com/llvm/llvm-project/issues/53475 )

You can break downstream users, the relationship inside the monorepo is different as far as I know: I believe we will have to live with revert like the one above until you collaborate actively to find a supported way and move there though.
Just asking "please move away from the library usage" alone won't be enough.

aganea added a comment.Feb 8 2022, 1:28 PM

@mehdi_amini Just to unblock users, I've posted https://reviews.llvm.org/D119277 which fixes the issue originally raised by @krzysz00
I'm also working on a more long term solution in https://reviews.llvm.org/D119049

Yes, I saw these, thanks a lot @aganea !!

(Since it looks like the discussion moved here) @MaskRay We can't spawn lld as a subprocess due to customer requirements around having the kernel generator/compiler delivered as a static library in some cases. (And there's LLVM version compatibility concerns, but those might be less of an issue in this case)

Having checked with other folks more familiar with the context of our project, it looks like we might actually be fine using lld as a subprocess, since we'll always be sitting on top of a ROCm installation, which includes lld, and since "take this object file and wrap it into a shared library" is extremely unlikely to be broken by version changes.

That being said, I've also heard that when this code was initially landing (as part of mlir-rocm-runner, and before my time), the feedback was to use lld as a library and not a subprocess.

Having checked with other folks more familiar with the context of our project, it looks like we might actually be fine using lld as a subprocess, since we'll always be sitting on top of a ROCm installation, which includes lld, and since "take this object file and wrap it into a shared library" is extremely unlikely to be broken by version changes.

Thanks for the flexibility :)

That being said, I've also heard that when this code was initially landing (as part of mlir-rocm-runner, and before my time), the feedback was to use lld as a library and not a subprocess.

I guess they did not realize how tricky to make lld/ELF library usage more reliable ...

krzysz00 abandoned this revision.Feb 10 2022, 9:48 AM

Worked around elsewhere, more permanent fix pending, abandoning

lld/MinGW/Driver.cpp