Sorry for the delay; yes this is still active.

Clean up the test.

In D75462#1902205, @jdoerfert wrote:

While I see how this is generally useful, I fear the implementation goes in the wrong direction.
What I mean is that we are moving away from instruction-based encodings for assume, see for example D74209.
What we need here is basically D73832.

@Tyker @lebedev.ri Maybe lifetime markers are the second use case for the "drop users" interface (D73404).

Fix CI issue ("no CHECK line").

Update the unit tests to work with the changes in this patch and the changes from upstream.

Indeed; discovered but only just before class. Thanks!

In D51984#1233719, @beanz wrote:

If you add this to your patch here, the Clang patch should work as I've suggested:

diff --git a/cmake/modules/LLVM-Config.cmake b/cmake/modules/LLVM-Config.cmake
index 8eabddc7377..a306e41f26d 100644
--- a/cmake/modules/LLVM-Config.cmake
+++ b/cmake/modules/LLVM-Config.cmake
@@ -244,6 +244,9 @@ function(llvm_map_components_to_libnames out_libs)
           list(APPEND expanded_components "LLVM${t}Info")
         endif()
       endforeach(t)
+    elseif( c STREQUAL "Polly")
+      # LLVMPolly is the Polly loadable module target, the static archive is just Polly
+      list(APPEND expanded_components "${c}")
     else( NOT idx LESS 0 )
       # Canonize the component name:
       string(TOUPPER "${c}" capitalized)

It is unfortunate that Polly doesn't match the naming conventions of other LLVM components, but we have a lot of special case handling for this kind of thing anyways.

In D51986#1232440, @beanz wrote:

After line 18 in this file you could do something like:

if(WITH_POLLY)
list(APPEND LLVM_LINK_COMPONENTS Polly)
endif()

Then you can get rid of the target_link_libraries call.

In D51986#1232440, @beanz wrote:

After line 18 in this file you could do something like:

if(WITH_POLLY)
list(APPEND LLVM_LINK_COMPONENTS Polly)
endif()

Then you can get rid of the target_link_libraries call.

In D51986#1232419, @DiamondLovesYou wrote:

In D51986#1232320, @beanz wrote:

I don’t think this is the right solution. The build system knows what components are in the dylib and should remove them from the list of libraries linked individually. You should be able to make Polly behave like an LLVM component, then tools don’t need to care if the dylib is used or not.

That's not true if target_link_libraries(${target} _whatever_ ${libs}) is used. That function will pull in all of each of ${libs}' dependencies, which causes problems because then, eg, bugpoint will then be linked with libLLVM.so (as expected) AND libPolly.a (assuming BUILD_SHARED_MODULES is false) AND then a bunch of LLVM components. The LLVM components (including Polly) will then conflict with libLLVM.so.

Polly already enjoys status as an LLVM component. No effort was necessary to include Polly in libLLVM.so for example.

In D51986#1232320, @beanz wrote:

I don’t think this is the right solution. The build system knows what components are in the dylib and should remove them from the list of libraries linked individually. You should be able to make Polly behave like an LLVM component, then tools don’t need to care if the dylib is used or not.

• Fix cmake warning

Fix cmake warning

See D51963.

Thanks for the comments, everyone. This was originally submitted to fix a rust-lang buildbot, which I've learned is based on CentOS 5.5 (yes). Anyhow, I've fixed the issue on that particular buildbot by defining PATH_MAX in C(XX)FLAGS. Thus this patch isn't needed anymore. Thanks again.

LGTM

LGTM

Anyway, I don't want to block the primary change with side debate, so forget this for now please.

In D15033#302259, @axw wrote:

In D15033#297984, @DiamondLovesYou wrote:

It looks like if LLVM is built with LLVM_LINK_LLVM_DYLIB=ON and --link-static is provided and the static archives are removed, llvm-config doesn't error and still prints the shared lib with --libs.

I've updated it to cater for this. It's a bit more complicated in order to keep current behaviour.

I'd prefer if we didn't do this "if static exists use, otherwise use shared", and rather just use whatever the installation is supposed to as according to LLVM_LINK_LLVM_DYLIB. Richard, if you have no objections, I'll change it to do that.

It looks like if LLVM is built with LLVM_LINK_LLVM_DYLIB=ON and --link-static is provided and the static archives are removed, llvm-config doesn't error and still prints the shared lib with --libs.

In D12338#245624, @jyasskin wrote:

In D12338#245398, @jfb wrote:

@jyasskin I took a look at implementing what we discussed, and it's quite a bit uglier than I though it would be:

• We want the same value that factors into ATOMIC_*_LOCK_FREE, which is MaxAtomicInlineWidth from tools/clang/lib/Basic/Targets.cpp (each Target should define a value there, or gets 0 by default).

Richard and I don't think you want that value. On x86-64, the maximum width here is actually 128 bits, but you want to split loads down to 64 bits.

• We can pass this to the backend through LLVM's TargetOptions, which clang's BackendUtil.cpp can initialize.
• AtomicExpandPass has a TargetMachine, which has TargetOptions.

This will work for code that come straight from C++, but won't work for code that only uses opt: clang has the information we need, and LLVM doesn't. This is pretty much something that should be in the DataLayout instead, which is a *very* involved change! WDYT? Am I missing something obvious?

You're probably right, but I don't know enough about the backends to be sure. It does seem like the backend should know the widths of atomics it supports.

Fixes for recent trunk changes.

Fix compile issue.

Mac puts the package revision before the extension.

In D13198#279173, @brad.king wrote:

Bump.

I've created a better patch similar to this one: http://reviews.llvm.org/D13198.

Bump. Could I get someone's attention to review this? I'd like to get it accepted.

Added tests for the other atomic orderings.

Fix comments.

Unconditionally make the initial load atomic and fix the resulting X86 test failures.

Make this patch do the Right Thing on Windows. Also fix a typo in a comment.

Addressed JF's feedback. Also made expandAtomicRMWToCmpXchg delegate NewLoaded && Successs value to the callback function.

I almost forgot, the original patch was here.

Restructure in response to JF's comments.

Fix a comment.

In D10349#190766, @echristo wrote:

Seems odd, why not just remove it since it's supposed to be "for a debugger"

(For the record, I have no dog in this show, just asking about the logic.)

-eric

Fixed JF's comments.

Fix use of an incremental diff.

Fix a comment from the last patchset.

Addressed JF's comments.

In D7713#125228, @t.p.northover wrote:

No tests?