In D138847#4186309, @gulfem wrote:

Overall, it looks good to me. I added a few minor things.
Please add --show-mcdc to the llvm-cov command guide.
https://github.com/llvm/llvm-project/blob/main/llvm/docs/CommandGuide/llvm-cov.rst

Update diff to fix a small bug and add a test case to test intrinsic generation (based off a comment in the backend review)

Updated diff based on review comments

Hi @ellis, thank you for looking at this deeper! I'll update based on your comments below, but I want to respond to your primary question first:

@peter.smith I know you may not feel comfortable reviewing everything or signing-off on the review, but for changes pertinent to clang, I think you made some substantive suggestions, so I am including you in the reviewer list. Thanks for your input.

Thank you for the comments thus-far. I have split this patch into three stacked patches and updated them according to comments found on this review:

Thank you (both) for looking at it! I appreciate your time.

In D135929#3886921, @gulfem wrote:

We would like to simplify our coverage pipeline by using debuginfod in llvm-cov. When we include binary ids into indexed profiles and start using debuginfod in llvm-cov

In D130786#3688503, @sunho wrote:

@alanphipps I just confirmed that the buildbot failures were fixed by the new fix https://reviews.llvm.org/rG65c9265f4158. Could you check out if this fixes the failure on your end too?

I'm still seeing a failure on my downstream Arm compiler on Linux in the unit tests -- I thought I saw the same failure on the buildbots:

Should there be an aarch64 guard on the test? There exist downstream Arm compilers that don't support Arm64 and fail. I know other tests explicitly state "REQUIRES: aarch64-registered-target"

In D127226#3575067, @alanphipps wrote:

Nope, it was working fine until a few hours ago. The only files I see actually installed in include/c++/v1/__type_traits/ are "integral_constant.h" and "is_callable.h".

In D127226#3574990, @philnik wrote:

add_lvalue_reference has been added in D126244, but I don't think there is anything different from the other granularization patches. Did anything fail like this before?

Hello! I'm noticing test failures with our downstream Arm compiler that may be related to the changes you're making, but I'm not sure where to begin looking.

We've also been seeing failures in our downstream Arm compiler when running the Perennial C++14 compliance tests related to this change. Specifically, the tests expect a diagnostic to be issued when the lambda capture variable is outside of the lambda's {} scope. Another tests uses the lambda capture variable in a decltype-style return type which is also outside of the scope.

There is no way to implement an "atomic" operation on an M0 besides turning off interrupts, and we don't want the builtins library to mess with the interrupt mask. I'd expect the OS/user to provide the functionality, if they need it. compiler-rt doesn't provide __sync_* for cortex-m0, for the same reason.

I'm not sure how your setup was working before. I guess maybe if you only need atomics for C++ static local vars, and you built libc++abi with LIBCXXABI_HAS_NO_THREADS? If you don't actually need threading/atomics, you can use -fno-threadsafe-statics and/or -mthread-model single.

In D120026#3424687, @efriedma wrote:

By "the builtins", do you mean you're using compiler-rt with -DCOMPILER_RT_EXCLUDE_ATOMIC_BUILTIN=Off? That implementation probably don't work on a Cortex-M0, unless you've hacked it somehow; it assumes width-1 lock-free atomics are available.

In D120026#3423066, @efriedma wrote:

The impact on Cortex-M0 is that instead of using __sync_*, and inlined atomic load/store ops, it uses __atomic_* calls. I can't imagine that causing timeouts unless you're somehow using a bad implementation of __atomic_*.

Interesting, this commit results in several testing timeouts in our downstream compiler for several C++ compliance tests (ACE, plumhall, perennial) for Cortex-M0/M0plus only. I confess I don't understand the nature of this commit or how it would impact runtime here, other than you indicate that this could impact Cortex-M0. Do you have any pointers as to what I could verify here?

Thanks for putting this up! The changes appear to be straightforward, and it makes sense to deprecate the older option and still accept it until next release. LGTM.

In D109958#3056240, @SjoerdMeijer wrote:

I assume you have a downstream change to enable it by default, just like us. So if we are going to enable this by default under the NPM, I am not sure if anything will change for you

There are already a good number of divergences between the two pass managers, I don't see the harm in keeping the LPM pipeline the same. Do you specifically care about this pass?

Thanks for the suggestion, I am happy to enable LoopFlatten by default only for the new PM, so will look into that.

This review appears to have stalled. Is anything missing as a result of the more recent comments to complete it? Thanks!

In D108358#2954926, @fetzerch wrote:

Thanks! This is my first contribution to LLVM and I don't have commit permissions. Could you or some of the maintainers please commit it on my behalf (Christian Fetzer <christian.fetzer@bmw.de>)? What would I need to do to 'block 12.x and 13.x release branches'?

Thank you for catching this! LGTM. Can you please ensure this defect fix blocks 12.x and 13.x release branches as well?

Hello! I'm seeing a failure from this commit in this test with our downstream Arm32 compiler. Does it ring any bells? Thanks.

In D99205#2802936, @alexey.zhikhar wrote:

In D99205#2792215, @alanphipps wrote:

Thanks for all of the effort! On our downstream Cortex-R5 compiler, I'm seeing a 20.4% speedup on Coremark with this patch, which is good, however, the older patch (https://reviews.llvm.org/D88307) gave me a 21.6% speedup. Any idea what could account for the difference there?

It would be very hard for us to explain the difference without having access to both the downstream compiler and the hardware but something that comes to mind is:

(1) were the two experiments performed on the same baseline compiler?
(2) 1% could be within noise and might be due to trivial changes in the generated code (change of alignment, change of branch addresses and branch target addresses, etc.)
(3) were both passes in the same place in the pipeline in both experiments?

From the information provided, it is not possible to understand the root cause of the difference.

Thanks for all of the effort! On our downstream Cortex-R5 compiler, I'm seeing a 20.4% speedup on Coremark with this patch, which is good, however, the older patch (https://reviews.llvm.org/D88307) gave me a 21.6% speedup. Any idea what could account for the difference there?

OK, I will revert that commit on the 12.x branch then. Can you please file a bug for this patch and mark it as blocker of the release-12.0.1 metabug? I typically review and try to get an ack from at least on other person before backporting patches.

In D102193#2752558, @tstellar wrote:

I saw this fix was backported to the release/12.x branch, was there a bug for this?

In D102193#2750971, @thakis wrote:

Looks like this breaks tests on windows: http://45.33.8.238/win/38516/step_11.txt

PTAL, and revert for now if it takes a while to fix.

In D96280#2611651, @alanphipps wrote:

We also encounter a build failure on the Mac related to above but in a different file:
clang/lib/CodeGen/CGStmtOpenMP.cpp:1916:3: error: too few template arguments for class template 'SmallVector'
SmallVector<llvm::Value *> EffectiveArgs;
^
clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here
template<typename T, unsigned N> class SmallVector;

In D94973#2617304, @Meinersbur wrote:

Is is that same problem that D98265 addresses?

Hello! I am seeing a downstream build failure on Mac as a result of your changes:

We also encounter a build failure on the Mac related to above but in a different file:
clang/lib/CodeGen/CGStmtOpenMP.cpp:1916:3: error: too few template arguments for class template 'SmallVector'
SmallVector<llvm::Value *> EffectiveArgs;
^
clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here
template<typename T, unsigned N> class SmallVector;

In D84467#2449045, @vsk wrote:

Unfortunately I don't think I'll be able to commit this on your behalf. Would you be open to obtaining commit access from Chris and landing this directly? I ask because, for a patch of this size, there's typically a fair amount of post-commit feedback (primarily in the form of bot failure notifications, but occasionally design feedback as well). It's common for tests to pass locally, but not on some specific CI node, and it can take quite a bit of time to resolve those issues.

In D88307#2345978, @eastig wrote:

Hi Alan,
Good news. I've got an approval. I cannot say when I post the updated version. I've just started a new job and are completely busy. I don't work anymore at Arm.

In D84467#2423636, @vsk wrote:

Thank you, lgtm!

Diff after rebase.

In rGa78aaa1ad51214b2e04f41762e76bb43067ea1fd#962691, @ldionne wrote:

Done in d67e58f23a823, thanks for reporting. However, I'm still curious to understand how you're building libc++. Are you passing -D_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION manually?

I can fix this locally to get our builds running, but is it possible that you could commit a fix upstream? I appreciate it, thanks!

In rGa78aaa1ad51214b2e04f41762e76bb43067ea1fd#962071, @ldionne wrote:

In rGa78aaa1ad51214b2e04f41762e76bb43067ea1fd#962068, @alanphipps wrote:

I am seeing downstream build errors for fallback_malloc.cpp with this commit, e.g.:

llvm-project/libcxxabi/src/fallback_malloc.cpp:237:10: error: no type named 'libcpp_aligned_free' in namespace 'std'
std::libcpp_aligned_free(ptr);
~~~~~^

How are you building the library? What's your configuration, system, compiler, etc? As you can see, our CI is green, so whatever you're experiencing is on a setup that we don't actively test.

In the description, you refer to fallback_malloc.cpp "The code in libcxxabi/src/fallback_malloc.cpp looks like it could be simplified after this change -- I purposefully did not simplify it further to keep this change as straightforward as possible, since it is touching very important parts of the library."

Looks like _LIBCPP_HAS_NO_THREADS is being set for libcxxabi, and the build now fails with this change:

Bump format version and add preliminary documentation for CoverageMappingFormat, SourceBasedCodeCoverage, and llvm-cov tool

Just checking in again -- is there interest in completing this review? I'd be happy to assist, or I could post an updated version of it.

@eastig I tried this experimental patch in our system, and it definitely brings highly significant improvement to coremark. When do you expect to post a version for reviewing -- what will the differences be?

Updated diff:
1.) Fix a defect in the CGExprScalar.cpp code to instrument RHS condition for branch coverage that resulted in a RHS condition as being evaluated twice. This is incorrect behavior since a condition may have side-effects. I also added a test.
2.) Back out unintended whitespace/punctuation (please let me know if I overlooked anything).
3.) Rebase against updated upstream.

In D84467#2264205, @vsk wrote:

@alanphipps thanks for bearing with me. I think this is about ready to land. I do ask that you back out any punctuation/whitespace changes in code/tests that aren't directly modified in your diff (the clang-format-diff script can help with this). It looks like some libCoverage and test changes are no longer in the current version of this patch, as compared to the previous version (https://reviews.llvm.org/D84467?id=287269). Was that intentional?