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?

There exist downstream ARM compilers that doesn't support ARM64 and fail with Inputs/arm_function_name.ll. You should consider guarding arm64 specific tests.

• Rename isLeafCondition() to isInstrumentedCondition() and rephrase associated comments
• Add branch regions on C++ range-based loops

I just ran into what I think may be the same issue here in that llvm-objdump seems to ignore machine data (endian, possible mode) when disassembling ARM instructions on ARMv7. Is there any interest in completing this review and committing this particular fix (and making sure it works for ARMv7 as well)?

Updating for formatting and comments (and some test adjustments after rebase). Bypassing logical-NOT operators in CodeGenFunction::isLeafCondition(), which checks the condition for the presence of logical-AND and logical-OR. (this can be further expanded, if necessary)

In D84467#2180421, @vsk wrote:

I haven't taken a close look at the tests yet, but plan on getting to it soon.

I'm not sure whether this is something you've already tried, but for this kind of change, it can be helpful to verify that a stage2 clang self-host with assertions enabled completes successfully. For coverage specifically, it's possible to do that by setting '-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On' during the stage2 cmake step.

In D84467#2180421, @vsk wrote:

I haven't taken a close look at the tests yet, but plan on getting to it soon.

I'm not sure whether this is something you've already tried, but for this kind of change, it can be helpful to verify that a stage2 clang self-host with assertions enabled completes successfully. For coverage specifically, it's possible to do that by setting '-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On' during the stage2 cmake step.

Thank you for your comments on the review! I will respond soon; also have been swamped this week.

It looks like clang/test/Profile/Inputs/c-general.profdata.v5 is being read as v6 rather than v5. Can you double check?

clang/test/CodeGen/sanitize-coverage.c is also failing our downstream embedded ARMv7 validations.

This test is still failing on the arm bots and also with my downstream ARM compiler validation.

Hello! Your test specifically looks for "248", which is failing with my downstream embedded ARM compiler (we produce "249"); other similar tests that I've looked at simply ignore this value. Is there a reason why you need to check it specifically?

Sorry, we just saw your commit to fix. We are OK. Thanks!

We maintain a downstream embedded ARM compiler, and this test is failing for us now because it assumes a 64bit size_t in the first CHECK line of the mempcpy-libcall.c. Because this is testing a GNU specific builtin, should it be specific to only targets that support that? Or should it be extended to support 32bit size_t?