User Details
- User Since
- Oct 27 2019, 11:03 AM (177 w, 4 d)
Mon, Mar 13
Dec 23 2022
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
Nov 29 2022
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:
Nov 28 2022
@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:
Oct 28 2022
Thank you (both) for looking at it! I appreciate your time.
Oct 27 2022
Oct 21 2022
Oct 20 2022
Oct 14 2022
Jul 29 2022
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:
Jul 25 2022
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"
Jun 11 2022
Jun 10 2022
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.
Apr 19 2022
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.
Apr 4 2022
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.
Apr 1 2022
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?
Nov 23 2021
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.
Oct 11 2021
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.
Oct 7 2021
This review appears to have stalled. Is anything missing as a result of the more recent comments to complete it? Thanks!
Sep 22 2021
Aug 20 2021
Aug 19 2021
Thank you for catching this! LGTM. Can you please ensure this defect fix blocks 12.x and 13.x release branches as well?
Aug 18 2021
Hello! I'm seeing a failure from this commit in this test with our downstream Arm32 compiler. Does it ring any bells? Thanks.
Jun 8 2021
Jun 1 2021
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?
May 18 2021
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.
May 11 2021
May 10 2021
Mar 10 2021
Hello! I am seeing a downstream build failure on Mac as a result of your changes:
Mar 8 2021
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;
Jan 12 2021
Jan 7 2021
Jan 5 2021
Dec 23 2020
Dec 11 2020
Diff after rebase.
Dec 2 2020
Nov 29 2020
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!
Nov 27 2020
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."
Nov 23 2020
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
Nov 19 2020
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.
Oct 19 2020
@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?
Oct 8 2020
Oct 3 2020
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.