Corrections are always welcome. Please submit an update to this thread (or a new patch) to fix the changes you propose.

If j-s doesn't exists (remember, most of that list came from the ancient days of llvm, so could very easily be completely wrong, but "works"), I all for removing it and making a new (correct) CPU name as the default for armv6k.

In D18086#1043635, @falstaff84 wrote:

I seem to have a related issue: I am using -march=armv6k and -no-integrated-as, this generates the following output:
/tmp/empty-bc2ea3.s:4: Error: unknown cpu `arm1176j-s'

Thanks Florian! LGTM too.

Very detailed, using the modern infrastructure and helpful even to those not using clang or wanting to run external benchmarks.

In D49563#1168689, @evandro wrote:

I shall, at least for Exynos, but I'd like to hear from our friends at ARM too.

Hi Takahiro,

Hi Evandro, looks great, short and simple!

Moved getMemInstAlignment and getMemInstAddressSpace to IR/Instructions.h which already contains similar helpers. Should I rename them to getLoadStoreAlignment and getLoadStoreAddressSpace to be more in line with the existing getLoadStorePointerOperand?

In D49488#1166644, @fhahn wrote:

Thanks for having a look so quickly! Maybe there is a better place to put it, maybe we should keep it local to lib/Transforms/Vectorize/? I initially put it in VectorUtils.h, because I wanted to avoid creating unnecessary new files (and splitting things up unnecessarily), but given that VectorUtils.h is used in quite a few places, I am happy to put it wherever it would fit best :)

I'll let Florian/Hideki reply about timeframes and strategies, and will just focus on specific items you list.

In D49491#1167024, @hsaito wrote:

The fact of the matter is that the loop vectorization has a need to understand SLP and SLP vectorizer needs to understand Loop. As such, unless we want to build/maintain separate LoopVectorize+SLP and SLPVectorize+Loop, consolidation of LoopVectorization and SLPVectorization will inevitably happen sooner or later. From that perspective, ensuring that VPlan is the right infrastructure for such consolidation is a very important thing for us to do.

My tuppence...

I like the idea of the interleave analysis to be at a higher ground, but we have to be careful with LV-specific logic and only hoist what it truly generic.

Thanks! I'm happy with Eli is happy. :)

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