In D65550#1752082, @vsk wrote:

Thanks Nick & Eli, '-fno-integrated-as' does the trick, as does changing the makefile. If the effect of this patch on the asm writer is intentional/correct, we can just push for a project change instead of fiddling with the compiler.

• Do not have a declaration as a prevailing symbol in llvm-lto2's arguments
• Fix comments in test

In D66814#1666487, @tejohnson wrote:

In D66814#1666457, @pirama wrote:

(note the summary description of the patch needs to be adjusted similarly, it still refers to the summary).

@tejohnson Did my reply about shared metadata clarify your question?

Check for OMPRT_Unknown instead of a default case.

Test -static, -static-openmp interaction. Added these only for iomp5 to avoid test-case explosion.

I'll update this review addressing @joerg's reply to cfe-commits:

Needs testing for the -static interaction?

In D67200#1660147, @srhines wrote:

Looks really nice. I am sure the NDK developers will be happy to see support for static OpenMP. Do you want to add the public NDK github issue link in the commit message?

Mention NDK issue https://github.com/android-ndk/ndk/issues/1028.

Does this regress from existing behavior for unused arguments? Currently, -static-libstdc++ -nostdlib issues an unused argument warning for -static-libstdc++, while AFAICT -static=c++stdlib -nostdlib doesn't.

Change parameter name.

Supported this flag for NetBSD and FreeBSD as well.

Update/clean-up comments in the test.

Fix typo reffers -> refers

Avoid extra variable

Thanks Peter. I'll wait for another day for further comments, and if none, will submit this tomorrow morning (pacific time).

In D65550#1610801, @efriedma wrote:

This only affects inline asm, it looks like, not general instructions? (I looked briefly, and it looks like instructions go through AArch64InstPrinter::printOperand instead.)

I don't think this would cause any issues off the top of my head, but you might want to run something like an Android build to confirm that.

Thanks for the workaround @peter.smith. I suggested it to the NDK developer as well. The immediate need for this change is gone, but I agree that this is useful for the various reasons you outlined.

Apply clang-format

Call strlen(gcov_prefix) only if its not null.

Remove stray new line.

Refactored the loop to call getenv just once.

LGTM for Android.

Skip version script for all MinGW builds, not just when using lld.

In D63743#1556912, @mstorsjo wrote:

I'd suggest skipping the lld condition, the version script adds no value here.

For ELF, version scripts do two things, select what symbols to export using glob pattetns, and set symbol versions on them.

As COFF doesn't have symbol versions, all it does on mingw with GNU ld, is filtering symbols, to export *. So it adds no value there.

In D54125#1313836, @void wrote:

Hi @piramam,

This change is breaking the Linux ThinLTO build. I'll work to get you a test case. Could you revert this patch until we can resolve the issue?

Someone just pointed out to me that even in Android P [1], libm's sincos() just called sin() followed by cos() so this optimization won't be beneficial. It was changed in March 2018 [2] but is not a part of any release yet.

In D54125#1291931, @smeenai wrote:

From the summary,

"In ELF, symbols non-weak symbols"

The first "symbols" seems extraneous?

Thanks for the review Teresa. I'm running the tests again after a rebase and will commit this after the tests pass.

Fix a few more comments.

Update based on review comments. Hopefully I did the WeakForLinker->Prevailing
name changes in all the right places.

Rename thinLTOResolveWeakForLinker* to thinLTOResolvePrevailing*

Fix commit message.

Adding reviewers suggested by 'arc cover'.

Simplify patch.

This file is not built for WIN32.

As I mentioned in the discussion, we decided to carry build rules for the proposed library in downstream. I've updated this to make it more general, and will leave it open in case there's more interest to revive it in the future.