D34472 is providing a more general solution

Some minor comments.

ping?

Add goldmount to llvm\test\CodeGen\X86\cpus.ll

LGTM

In D32658#788057, @Farhana wrote:

Hi Simon,

Do you have further comments/concerns?

Farhana

• test/CodeGen/X86/clear_upper_vector_element_bits.ll - DAG is improved, but scheduling causes a code size degradation. This is probably okay.

@t.p.northover Are you happy for the AArch64 codegen regression to be raised as a bug?

In D34141#786536, @delena wrote:

In D34141#786514, @RKSimon wrote:

While I don't want to end up forcing you to over-engineer this, it seems this pass is currently very specific and should be generalised to make it straightforward to work for constants arrays of other patterns that are easily re-materializable (upper/lower masks, single bit masks, etc.) - "SubstituteLoadWithRematerializationPass", possibly driven by the TTI cost models?

I agree that the name of the pass should be more generic.
Do you mean renaming by "generalization" ? Or you propose to add this transformation for all targets and ask TTI about profitability of this transformation?

LGTM. Please do the peekThroughBitcast helper as a NFC pre-commit.

While I don't want to end up forcing you to over-engineer this, it seems this pass is currently very specific and should be generalised to make it straightforward to work for constants arrays of other patterns that are easily re-materializable (upper/lower masks, single bit masks, etc.) - "SubstituteLoadWithRematerializationPass", possibly driven by the TTI cost models?

Does anyone have any concerns about this? Otherwise I'd like to accept it.

LGTM, with the addition of some ctpop vector tests, even if they don't currently combine.

LGTM with one minor about the comment in the lv test

Any chance of a test case?

Adding @chandlerc who refactored this code at rL214623

In D33840#771551, @RKSimon wrote:

Do you have any stats?

In D34336#783558, @craig.topper wrote:

vpternlog does not have any idiom recognition.

For pcmpeq I think intel only avoids the dependency but still executes it. What does AMD do?

Some minor observations. It'd be great it we could reduce the size of this patch some how....

@aymanmus I'm sorry but I had to revert this at rL305470 as it's causing tablegen crashes on windows buildbots: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/10612

Updated as requested by @chandlerc - my only concern is I have no idea if uniform_int_distribution guarantees the same behaviour on different targets as mersene does.

In D34089#779145, @mclow.lists wrote:

Is there a C++11 random class that covers the features we need (same results on different targets, drivable by input seed)?

Given that the libc++ mersenne_twister_engine tests test specifically that, yes.

see <libc++ root>/test/std/numerics/rand/rand.eng/rand.eng.mers/ctor_sseq.pass.cpp

In D33983#779990, @magabari wrote:

In D33983#777512, @RKSimon wrote:

I've added the SLP vectorization tests at rL305151

Simon I saw that you have added that to the tests.
Is there is a need to do something else?

LGTM

In D34089#779023, @mclow.lists wrote:

After thinking about this, I agree with @bogner 's comment that we should just remove the whole Random class, and use the C++11 random number facilities instead.

In D34087#777942, @niravd wrote:

It looks like this is really an issue of "areNonVolatileConsecutiveLoads" not being clever enough. BaseIndexOffset in DAGCombiner should do better. I'd favor moving BaseIndexOffset out of the DAG Combiner and rewriting areNonVolatileConsecutiveLoads off of that.

Added assertion that the random value doesn't exceed the maximum.

In D34087#777942, @niravd wrote:

Also, popping up a level, do you know if there's a reason why this is done as an X86-specific pass? It seems entirely generic.

A few minors, but I'd prefer someone more knowledgable in this area to review it.

Rebaing - this is part of constant canonicalizations to try and combine MUL/SHL ops separated by ADD/OR ops.

I've added the SLP vectorization tests at rL305151

Looking at lowerV2X128VectorShuffle, shuffle combining will have a much easier time if we keep to 256-bit vectors (blends / X86ISD::VPERM2X128) as much as possible - subvector extract/insert chains makes combining really tricky - and this dealing with memory cases looks like a good first step.

If possible I'd like to see a regular llc test as well as the mir test.

A few minors, but would like to see ARM fixed first.

ping?

LGTM

A couple of NFC changes that will reduce this patch.

@ABataev You marked a lot of the comments done but haven't updated the diff.

D33897 is moving SNB/HW scheduler table to auto-gen

In D33983#775010, @magabari wrote:

In D33983#774990, @RKSimon wrote:

You might want to add a slm target to the llvm\test\Transforms\SLPVectorizer\X86\arith-add.ll, arith-mul.ll and arith-sub.ll tests as well.

You mean to split the slm-arith-costs.ll test to those files or to add extra test cases via checking those tests on SLM also?

LGTM

You might want to add a slm target to the llvm\test\Transforms\SLPVectorizer\X86\arith-add.ll, arith-mul.ll and arith-sub.ll tests as well.

Please don't forget to include llvm-commits as a subscriber to all llvm patches

Almost there I think - couple of minors