LGTM.

In D72399#1822902, @rnk wrote:

In D72399#1821475, @russell.gallop wrote:

Honestly, if __thread is available, I wonder if LLVM should prefer that, since it implements the check that the variable is trivially constructible/destructible. But, that has no bearing on which declspec to use.

Is "trivially constructible/destructible" actually a requirement for this? I thought that restriction was due to host platforms which didn't support C++11 thread_local (hence if/when every platform supported C++11 thread_local then the restriction would go away).

Yes, LLVM still supports platforms which do not support C++11 thread_local, so for LLVM, there is still a requirement that things be trivially constructible/destructible. If we used __thread when available, the compiler would enforce that check earlier, rather than letting the contributor commit, push, and find out about this requirement from a buildbot.

In D71219#1776085, @rjmccall wrote:

Yeah, the discussion Jin and I had privately was that we should split the Swift ABI version out of the ObjC GC global metadata and update the backend so that it assembles the image info appropriately from different places. I don't know what happened to that.

That will probably require an IR updater, but that shouldn't be a problem.

In D69825#1771300, @arphaman wrote:

I guess Duncan's comments on https://reviews.llvm.org/D70568 were for this patch. I think it would be good to have:

In D70568#1768138, @dexonsmith wrote:

In D70568#1768129, @dexonsmith wrote:

In D70568#1768129, @dexonsmith wrote:

Can we make this configurable somehow? (E.g., leave behind an -ffork-cc1 -fno-fork-cc1 and a CMake flag to pick? Or just the CMake flag?)

This is really cool; we've wanted this for a long time.

LGTM, although I have a suggested change for one of the comments (and I'm still sad about this).

Pushed as 20d51b2f14ac4488f684f8fc57cb0ba718a6b91d.

Pushed as 20d51b2f14ac4488f684f8fc57cb0ba718a6b91d.

For some reason this revision is locked down and I'm not allowed to "edit" it, which includes adding inline review comments. Can you add me as a reviewer?

In D70285#1752361, @dexonsmith wrote:

In D70285#1752164, @Bigcheese wrote:

lgtm as long as other compilers don't warn on unknown pragmas by default.

godbolt.org says that GCC doesn't have -Wunknown-pragmas by default, but it is in -Wall. I'd better guard this more closely with __clang__.

In D70285#1752164, @Bigcheese wrote:

lgtm as long as other compilers don't warn on unknown pragmas by default.

Pushed as 3279724905c14a8db383ade53af40a0dd49504d8.

@rjmccall gave me an LGTM offline.

In D70370#1750591, @dexonsmith wrote:

In D70370#1750378, @ahatanak wrote:

It wasn't clear to me, but is the pair deleted only if there are no instructions that return false for isSafeBetweenRVCalls in between?

I thought it was okay to skip over all opaque calls, but I've changed the code to check for this.

Respond to Akira's feedback.

In D70370#1750378, @ahatanak wrote:

It wasn't clear to me, but is the pair deleted only if there are no instructions that return false for isSafeBetweenRVCalls in between?

Pushed as d4e1ba3fa9dfec2613bdcc7db0b58dea490c56b1.

Adding more testcases. The bitcast tests uncovered problems with the RAUW calls, which I also fixed.

Committed in 783cb86b616d9de59213ea17649d6e2df8c1ebbb.

Added an assertion for clarity and fixed a comment.

s/C_STRING_PROTOTYPES_/C_STRICT_PROTOTYPES_/ to fix some typos.

Adding guards for _MSC_VER.

Note that we have C files including these headers in llvm/tools/llvm-c-test and clang/tools/c-index-test, and I tested that dropping a (void) to just () triggers the error when compiling those.

The code looks right, but can you add a test for this in CodeGenObjC? Also, when you re-upload the patch with the tests, please use -U9999999 to provide full context in the diff.

Stepping back to look at the goals:

In D11833#1744718, @mehdi_amini wrote:

OK but these NDEBUG aren't necessarily harmful, are they?

IIRC we added LLVM_ENABLE_ABI_BREAKING_CHECKS to cover the cases because previous using NDEBUG in the client was *required* to match the settings at the time LLVM was built.

Hi Dave, thanks for checking in on this, but unfortunately we're not ready for this yet.

Committed 83dcb34b6bf4c175040b18d3e8c3c468418009fc. Thanks for the reviews!

In D69959#1741456, @rnk wrote:

What do you think of making FrontendOptions::Inputs be a SmallVector<*, 0>? It's janky and inconsistent with the rest of that struct, but it seems less invasive in the end.

Committed c46b3a2abd38d6fecd389c97dfa7df54af77fdb9.

Committed 8e2c192e2af8c760152ba3b28e774dbb1548e4aa.

Committed in bfd58fc60ff4b0c081b5b489119c3798d3e2b53c and 01782c3e4df1830d7991e9edfee9119ed41e4c27.

Committed as eef69021607950487a9e4110851a05abb54d0fb6.

Looks like I forgot to close when recommitting in r337514.

Updated to use a new diagnostic (err_module_file_missing_definition) that includes the filename of the PCM.

Updated header docs for the new delayed diagnostic parameter.