Adding some reviewers + folks on the original review:

In D46283#1118926, @zatrazz wrote:

401.bzip2	-2.04

I will check if 401.bzip2 slight drop is just noise or something related to this patch, but regardless I do think this change should yield better performance in most scenarios.

I'm guessing this is a fix for: http://lab.llvm.org:8011/builders/clang-cmake-thumbv8-full-sh/builds/185

SPEC06 results look too noisy to conclude anything, especially bzip, xalan and povray. Can you find a more stable machine?

In D46714#1098954, @kristof.beyls wrote:

It does seem like a wiki would be nice to maintain this kind of information. In the absence of that, I think that a file in the test-suite repository, or a page in www are about equally easy/hard to maintain: it requires commit access to make any changes.
A file in www in theory could be more visible as it becomes part of the llvm.org web pages. That being said, source code is also viewable online, so it's easy to browse this text too.

In D46714#1095034, @Meinersbur wrote:

I should have clarified: Regarding SPEC, I meant adding CMakeLists in the External directory.

In D46714#1095029, @MatzeB wrote:

It's odd to have this in the repository, but admittedly we don't really have a wiki or similar in LLVM so I may be ok.

We can't add SPEC, as it's commercial. I'm not sure about others, but please make sure they are open source.

LGTM with the line removed. :)

In D46010#1078477, @zatrazz wrote:

I should not change other architectures than AArch64 because 'isArithmeticDivFast' is a new method (only used by this patch).

Nice catch! Sorry for the delay, LGTM.

In D46278#1083413, @efriedma wrote:

It's not really csel vs. mov; the COPY likely gets coalesced away, and it might allow erasing the condition which feeds the select, which might allow erasing more code, etc.

In D46283#1083378, @zatrazz wrote:

[1] [llvm] r305960 - Enable vectorizer-maximize-bandwidth by default. (Dehao)
[llvm] r305990 - Revert "Enable vectorizer-maximize-bandwidth by default." (Diana Picus)
[llvm] r306336 - Enable vectorizer-maximize-bandwidth by default. (Dehao)
[llvm] r306344 - revert r306336 for breaking ppc test. (Dehao)
[llvm] r306473 - re-commit r306336: Enable vectorizer-maximize-bandwidth by default. (Dehao)
[llvm] r306792 - Revert "r306473 - re-commit r306336: Enable vectorizer-maximize-bandwidth by default." (Daniel)
[llvm] r306933 - Enable vectorizer-maximize-bandwidth by default.
[llvm] r306934 - revert r306336 for breaking ppc test.
[llvm] r306935 - re-commit r306336: Enable vectorizer-maximize-bandwidth by default.
[llvm] r306936 - Revert "r306473 - re-commit r306336: Enable vectorizer-maximize-bandwidth by default."

This does look like a heavy hammer for a small fix. From the optimisation guides, CSEL and MOV have the same latency/bandwidth and the condition is probably pipelined in anyway.

This seems like an improvement, but we have to be careful with wide variations and little gain. The geomean is almost null but the standard deviation is higher than 75% of the results.

In D45552#1082996, @hsaito wrote:

There seems like a disconnect.

In D46254#1083185, @hsaito wrote:

We need to consciously try using (or even creating) generically reusable code like this, instead of each file having it's own "skip this, skip that".

In D45552#1082936, @hsaito wrote:

For example, in loop fusion scenarios, doing this is not a rocket science.

makes sense. thanks!

In D45552#1082360, @hsaito wrote:

In this case, the most immediate client for us is actually our internal fast-track VPLan vectorizer (it's outside of LV so that we don't mess up LV accidentally),

In D45552#1082126, @hsaito wrote:

A public header has zero impact in compile time for the files that are not including it, for obvious reasons.

LGTM, thanks!

Do we have similar cost tests for x86, Arm and others? It would be nice to make sure the change to BasicTTIImplBase didn't break other targets' costs.

Thank you! LGTM!

Can't remember if it was @olista01 or @SjoerdMeijer who was doing the partial match in the assembler, but the idea sounds good to me. I'll let them (or someone else closer to the changes) to approve.

These patches all look very similar, I think they could be all in the same commit? Even being large, may be easier to see how they are similar and do a quick review.

In D45420#1070587, @hsaito wrote:

Side effect of that is D45552 will have to land in Transform first and then move to Analysis when the descriptors move.

Adding TableGen's code owner.

Adding Diego and Florian. We just had a good chat about this and maybe keeping the headers inside Transform for now would be better for the changes to come. We can move it later when it becomes clearer what the purpose is.

So, I never liked LoopUtils and I agree a lot of it should go to Analysis but having two copies of LoopUtils is bound to confuse people when including the headers.

Minus the whitespace change, LGTM. Thanks!

Minus the whitespace change, LGTM. Thanks!

Mechanical changes, LGTM. Thanks!

Right, pure code movement, and a good clean up at that, LGTM.

LGTM, thanks!

Hi Hideki,

LTM, thanks!

Mechanical update, looks good, thanks!

LGTM.

Simple and NFC. LGTM. Thanks!

Indeed, nice cleanup. I think we discussed this earlier and it's a much welcome change, to merge NEON and SVE parsing.

Hi Hideki,

For now, that the shuffle reduction is in loop utils, this patch is fine. But this really ought to be in target transform info.

LTO is something I never considered before in the context of the target parser, but I understand the issues are similar to what the build attributes were trying to solve, so adding more info shouldn't make it worse.

I had seen the first version of this patch and thought it confusing, so refrained from reviewing (too broad context), but this version is much clearer now, and I can see it as straightforward benefit.

In D44853#1047492, @mgrang wrote:

This patch is one of a series of patches to replace *all* std::sort to llvm::sort in llvm/clang/polly (and other tools). I have a patch (https://reviews.llvm.org/D44363) which replaces all std::sort to llvm::sort. But as per review comments, it was suggested to break down that patch target-wise to smaller chunks. Hence I am now pushing smaller target-specific patches.

In D42447#1033730, @dcaballe wrote:

Is -vplan-build-stress-test flag in Patch #3 (D44338) aligned with what you had in mind? :)

Nice! Great for bisecting! I'll let @maxim-kuvyrkov approve, as he's the one taking care of the Buildbots now.

I don't like this being on by default. I get it it uses EXPENSIVE_CHECKS, but still, most developers use Debug/Asserts builds.

LGTM, thanks!

LGTM, thanks!

LGTM, thanks!

Nice cleanup!

The original code was calculating the unroll factor by NumInstructions like:

Hi Mathew,

Can't this be done in the DAGCombine?

Right, now it's clear what it's doing, thanks! I'm happy with it, LGTM.

Right, this makes a lot more sense! :)

r327173

In D43971#1032615, @mstorsjo wrote:

No, that's probably because it aarch64-windows didn't exist at all back then (there's even no publicly available hardware yet), so this just did something else (looks like ELF relocations).

LGTM after D43314 goes in. Thanks!

LGTM with the sorting comment. Thanks!

In D43971#1031818, @mstorsjo wrote:

First: Currently there's the pseudoinstruction LOADgot which lowers into a "adrp + ldr" instruction pair, but the ldr always loads a full X-register. Here I needed to do that, but load a W-register instead, to only read 32 bits. I tried to do this by adding a separate LOADgot32 pseudoinstruction, but I wasn't able to make it return a 32 bit value on the SelectionDAG level.

Hi Hideki,

r327109

I agree this is a nice cleanup, too.

I think it should be fine if it's not in the list of patterns recognised as tests...

LGTM, thanks!

I don't know much about COFF TLS to tell why you need the special treatment, but the AArch64 asm looks correct (though very inefficient, but you know that:).

Fair enough, I can't see what case this is trying to cover anyway. I'm happy with the line going out without a big comment, commit message is usually ok.

Nice! LGTM, thanks!

In D43314#1021294, @homerdin wrote:

Tried some things and was able to get it working using /dev/stdout.