I tested this change on Graviton2 aarch64-linux with clang -moutline-atomics.
clang was configured with compiler-rt:

cmake -v -G Ninja \
  -DCLANG_DEFAULT_RTLIB:STRING=compiler-rt \
  -DLLVM_ENABLE_PROJECTS:STRING="clang;compiler-rt;libunwind" \
  -DCLANG_DEFAULT_UNWINDLIB:STRING=libunwind \
  -DCMAKE_BUILD_TYPE:STRING=Release \
  -DCMAKE_INSTALL_PREFIX:PATH=/home/ubuntu/llvm/usr/ \
  ../llvm

I tested this change on Graviton2 aarch64-linux by building https://github.com/xianyi/OpenBLAS with clang -O3 -moutline-atomics and make test: all tests pass with and without outline-atomics.
Clang was configured to use libgcc.

This cleanup looks good to me.

Looks good to me. Thanks!

Ping.

I looked at both the SLP and loop vectorizer and I think this is more work than I can do right now.

In D67645#1674197, @az wrote:

There are few things missing in current work such as indexed dot product or what they call s/udot (vector, by element) in the ARM document (no need to do it now but a comment about that would help).

In D67645#1673951, @SjoerdMeijer wrote:

to do the heavy lifting, this is probably a task for the loop vectorizer.

To catch more dot product cases, we need to fix the passes above instruction selection.

In D67645#1673114, @SjoerdMeijer wrote:

• was just curious about the AddedComplexity = 30

In D67645#1672161, @SjoerdMeijer wrote:

I've got a cheeky request, and I appreciate that should go in a separate patch, but while you're at at it would you mind repeating this exercise for the ARM backend and AArch32?

The new patch does not use the first argument of the dot product instruction: we now set it to zero.
Patch tested on x86_64-apple-darwin with make check-all.

Excellent!
Thanks for catching those patterns.
Please commit.

I still see a link error on aarch64-linux on master:

/usr/bin/ld: tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/AttrImpl.cpp.o: in function `clang::AttributeCommonInfo::getAttributeSpellingListIndex() const':
/home/ubuntu/llvm-project/llvm/tools/clang/include/clang/Basic/AttributeCommonInfo.h:166: undefined reference to `clang::AttributeCommonInfo::calculateAttributeSpellingListIndex() const'
collect2: error: ld returned 1 exit status

I reverted locally this patch and it finishes building.

In D67497#1668236, @SjoerdMeijer wrote:

The patch looks okay to me, but I am still curious what happens with i16. The lowering to umov w8, v0.h[1] in build-vector-extract.ll is probably the interesting one. This is probably covered by rule:

Looks good to me, please apply.

Updated patch by removing the patterns that generate i16. Patch passes make check-all on aarch64-linux.

The tablegen error only happens on the i16 patterns:

def : Pat<(i16 (extractelt (v8i16 V128:$V), (i64 0))), (EXTRACT_SUBREG V128:$V, hsub)>;
def : Pat<(i16 (extractelt (v4i16 V64:$V), (i64 0))), (EXTRACT_SUBREG V64:$V, hsub)>;

If I remove these two patterns, make check-all passes.

In D67497#1667851, @evandro wrote:

Is there a test case that checks that this change does not break what the code in AArch64DAGToDAGISel::Select() was meant to handle?

Never mind. You cannot do the other ones as it would call Match too many times and would not follow the semantics of the original code.

Almost LGTM.

I like the patch. Thanks!

Updated patch to current llvm trunk.

Ping patch.
The last version of the patch addresses all the comments from reviews.
Ok to commit?

For some reason asan/tests/asan_noinst_test.cc is not compiled on make check-asan and that has exposed a compile error that was not triggered by the other tests:

sanitizer_double_allocator.h:30:11: error: use of non-static data member 'use_first_' of 'DoubleAllocator' from nested type 'DoubleAllocatorCache'
      if (use_first_)
          ^~~~~~~~~~

The updated patch fixes this by accessing the non-static field of the enclosing class through a this pointer to one of the instances:

-      if (use_first_)
+      if (this->use_first_)

The updated patch passes make check-lsan check-asan and is still under test for check-all on aarch64-linux.

The updated patch I will post is addressing all the review comments.

Addressed comments from @vitalybuka: factored up the 3 versions and added more tests.
Passes with no new fails ninja check-all on an AArch64 Graviton A1 instance.

Fix the x86_64 overflow warning with 1ULL << 48.

Updated patch fixes ASan.

I have verified that the updated patch compiles and it reduces the execution time of leak sanitized trivial example.

This patch reduces the number of #ifdefs as suggested by Kostya, and speeds up both the leak and address sanitizers on aarch64.
Passes check-all on x86_64-linux and aarch64-linux is still under test.
Worked with Brian Rzycki @brzycki.

Also, this is changing only the standalone lsan, not lsan used as part of asan. Right?
standalone lsan is not widely used, AFAICT.

Address review comments from Kostya: move AArch64 lsan allocator to a separate file to avoid #ifdefs.

Ok, I will prepare an updated patch.
Thanks Brian and Kostya for your reviews.

Looks good to me.

Rebased patch on today's trunk.

In D32530#1452773, @rovka wrote:

In D32530#1451505, @lebedev.ri wrote:

I'd advise caution here, it's really significant/impactful change, and a single sign-off is a bit worrying.

I am accepting the scalable vector types based on the comments in
http://lists.llvm.org/pipermail/llvm-dev/2019-March/131137.html

This seems to be a useful transform that is not yet covered by the current implementation of jump-threading.
I think GCC calls it control dependence DCE.
Please run and report performance numbers on the testsuite and other benchmarks that you have access to.

Maybe you can add the testcase from my previous patch: https://reviews.llvm.org/D53588

Looks good to me.

Ok, thanks!

In D53588#1276693, @vsk wrote:

@sebpop are you interested in rebasing this on the new cold block propagation code? If not, I'd be happy to give it a try. Is the main challenge in teaching CodeExtractor to update the DT and PDT?

The change looks good to me. Thanks!

ok

ok

The change looks good with some minor changes. Thanks!

In D53588#1272789, @vsk wrote:

@sebpop thanks for this patch! I don't see any problems with it (although I would prefer that the test explicitly check that outlined functions contain the correct instructions).

At a higher-level, I'm seeing some problems with the forward/back cold propagation done in getHotBlocks on internal projects. The propagation seems to stop when it encounters simple control flow, like an if-then-else or a for loop, after which cold code is unconditionally executed.

I have a prototype of a different propagation scheme which overcomes some of these limitations. The idea is to mark blocks which are post-dominated by a cold block, or are dominated by a cold block, as cold. This is able to handle the control flow I described, and isn't limited to requiring a single exit block. Could you give me a day to evaluate it further, run benchmarks etc. and report back? If it turns out to be promising, istm that it'd make sense to rebase this patch on top of it.

ok

ok

ok

ok

ok

In D52904#1266032, @NoQ wrote:

There seem to be more buildbot issues, eg. http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-incremental/54221/console

Fixed in https://reviews.llvm.org/rL344566

In D52904#1265893, @hiraditya wrote:

With this patch I now see a 3% speedup on sqlite vs. no hot-cold-split pass.

Awesome! Thanks for improving this!

In D52904#1265841, @sebpop wrote:

Are both fixes necessary to fix the issue (the one for back propagation and the one to bail out if the entry block is cold), or is either one sufficient? The patch description only mentions the former.

Are both fixes necessary to fix the issue (the one for back propagation and the one to bail out if the entry block is cold), or is either one sufficient? The patch description only mentions the former.

Fix two comments from @tejohnson.

I will post a patch to fix the comments from @tejohnson.

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

Added an early return for outlining if function entry is cold.
Added a check for invoke calls: invoke should not be marked as cold by back propagation.

Before this patch we have a 10% regression in sqlite with hot-cold-split pass.
With this patch I now see a 3% speedup on sqlite vs. no hot-cold-split pass.

Looks good.

Please add a testcase that will exercise the new flag.

In D50658#1247050, @tejohnson wrote:

In D50658#1240509, @tejohnson wrote:

I see that there is a new Pass Manager version of your new pass, but I don't see that it is ever being enabled in the new pass manager pipeline (or tested). Are you planning to add that?

Pinging question. It should be straightforward to add to the new PM, is that something you plan to do soon as a follow-on?

lgtm

Looks good to me.

I broke the check in https://reviews.llvm.org/rL341838 and fixed it in https://reviews.llvm.org/rL341839

Looks good to me.

The patch looks good to me.
Please address the last two comments and then apply.

lgtm, thanks!