I checked this on my arm64 mac:

• These test are already failing on main branch due to the mismatch with -DCOMPILER_RT_HAS_FLOAT16
• They are still failing with this patch (due to conditionalizing the macro definition on x86)
• If I remove the conditional and always define COMPILER_RT_HAS_FLOAT16 when building the builtins for darwin, the tests pass for me. I don't know which ABI we want here for arm[64], but it confirms we need to be consistent about the conditional define.

Reverted in eab2a06f0fde due to the Darwin test failures.

This broke some compiler-rt tests on Darwin:
https://green.lab.llvm.org/green/job/clang-stage1-RA/29920/

Couple more minor things, but basically LGTM.

Removed use of std::unique_ptr in DependencyScanningTool.cpp, per review feedback.

Attempt to fix Windows path issue in test.

LGTM, although I made a suggestion about the spelling.

Minor suggestion for the test case, but otherwise LGTM.

Does check-orc-rt need to be added to COMPILER_RT_TEST_SUITES too? I guess not, otherwise we wouldn't have seen this missing dependency in the first place?

Make sure the test failure gets fixed, but otherwise LGTM.

x64 debian > LLVM-Unit.Support/_/SupportTests::VFSFromYAMLTest.RelativePaths

Each VFS may have its own working directory, so it could be surprising if we use the OS working directory instead of that. This is complicated by the fact the VFS working directory may not be set yet during parsing the yaml (I haven't checked). I'm not really sure what to recommend here. If we do change this, we should document the new behaviour in the doc comment for RedirectingFileSystem though.

I don't know, I'm a bit skeptical we want to make it so easy to ignore errors so easily. I'd rather require clients to explicitly ignore the error.

Handling "" as enabling all logging is a bit strange to me -- what was the reasoning for that? LGTM other than than the return value from initializeDebug that I mentioned.

Updated to enable the visibility macro when using BUILD_SHARED_LIBS and remove the MLIR-specific change, per review.

Never mind, I got it working on Linux. BUILD_SHARED_LIBS of MLIR doesn't seem to work correctly on macOS. New patch with a fix: https://reviews.llvm.org/D115825

@mehdi_amini x86_64 macOS

@mehdi_amini I'm seeing the same failure output in the llvm.c test even with my changes reverted and a clean build. Any idea what I might be missing? I am building with the following arguments -- they're about as close as I could get to that bot without having CUDA.

@mehdi_amini thanks!

Since the new approach is no longer "opt-in", I tested:

• Default build (static, LLVMInitialize* is external)
• -DCMAKE_CXX_VISIBILITY_PRESET=hidden (static, LLVMInitialize* is hidden -- this is the new thing)
• -DBUILD_SHARED_LIBS=YES (dynamic, LLVMInitialize* is external due to not overriding visibility)
• -LLVM_LINK_LLVM_DYLIB=YES (dynamic, LLVMInitialize* is external due to LLVM_EXTERNAL_VISIBILITY)

Updated to set LLVM_EXTERNAL_VISIBILITY only when building the LLVM dylib.

In D113610#3183892, @ldionne wrote:

I don't have a horse in this race since this is LLVM and not libc++, however from a user perspective, it would feel more ergonomic to have a single setting like LLVM_VISIBILITY=normal|hidden. Or something like LLVM_HIDDEN_VISIBILITY=ON (by default it's OFF, which means default visibility is used).

Heck, thinking about it, why does LLVM even override what's decided by CMAKE_CXX_VISIBILITY_PRESET? It seems to me that:

• LLVM_LIBRARY_VISIBILITY should always be hidden, and
• LLVM_EXTERNAL_VISIBILITY should be whatever CMAKE_CXX_VISIBILITY_PRESET says

Ping, does anyone have any feedback on this?

4c94760f360bd4e24519a61bea3bde0413aaf78a

Part of me wants to say that we should make these visibility macros conditional on the existing LLVM_ENABLE_DYLIB (sp?) option. If the user is statically linking LLVM, we should drop these annotations and allow the global visibility preset to take precedence.

but maybe linking more than one copy of LLVM is rare enough that the issue hasn't come up before?

It looks like this caused an assertion failure on buildbots, would you mind reverting?