In D94779#2515157, @dmgreen wrote:

Thanks. @fhahn @SjoerdMeijer what do we think about the edge case where the width==1? As far as I understand (with this patch):

#pragma clang loop vectorize_predicate(disable) vectorize_width(4)
Gives llvm.loop.vectorize.predicate.enable=false, llvm.loop.vectorize.width=4, llvm.loop.vectorize.scalable.enable=false, llvm.loop.vectorize.enable=true

#pragma clang loop vectorize_predicate(disable) vectorize_width(4)

I guess this is a typo, and should be vectorize_predicate(enable)

Gives llvm.loop.vectorize.predicate.enable=true, llvm.loop.vectorize.width=4, llvm.loop.vectorize.scalable.enable=false, llvm.loop.vectorize.enable=true

#pragma clang loop vectorize_predicate(enable) vectorize_width(1)
Gives llvm.loop.vectorize.width=1, llvm.loop.vectorize.scalable.enable=false

#pragma clang loop vectorize_predicate(disable) vectorize_width(1)
Gives llvm.loop.vectorize.width=1, llvm.loop.vectorize.scalable.enable=false

Should we be adding llvm.loop.vectorize.predicate.enable metadata even when width=1? I would be inclined to emit the predication pragma anyway.

Thanks @shchenz, comments addressed.

Thanks, LGTM too.

In D94457#2491639, @porglezomp wrote:

The existing fusion tests seem a little bit weak—the machine scheduler with the base Cortex-A57 machine models that most of those use seemed to pass the address and literal fusion on its own (by coincidence?) without the explicit features requested. I was unable to make that test case stronger to see the A57 machine model not exploiting those fusion pairs when the feature was disabled. Is that worth worrying about?

• Ported over the only positive test test/CodeGen/X86/sink-cheap-instructions.ll to a MIR test: this is sink_add in the new MIR test file.
• Walk candidates in reverse order, to start at the bottom of def-use chains, and try using the uses first before attempting to sink a def.

Looks reasonable.

• Added statistics NumLoopSunk, the number of instructions sunk into a loop
• Check that a use of candidate is in a loop,
• Added a bunch of negative tests.

In D94779#2504519, @malharJ wrote:

I had a look at the Clang Language Extension ... and I saw this:

Specifying a width/count of 1 disables the optimization, and is equivalent to vectorize(disable) or interleave(disable).

Ah yes, thanks. I think we should improve the langref spec also, this should be described in the same paragraph, not floating below some example.

I am trying to remember details here, but first about this:

Thank you very much for looking at this @shchenz !

Looks like we get a lot of happiness from using these attributes. ;-)
I had a look at it before and the general direction also looked okay to me too, which I was happy to see confirmed. Therefore I am happy to accept this, even though I haven't checked all intrinsics because this is a lot of changes. So perhaps wait a day with committing in case anyone wants to have one more look.

Yep, don't think it is useful or necessary to run this at O0.
LGTM.

I was going to look at this today, but thanks for fixing! :-)
LGTM

To avoid code-size bloat, this is indeed the behaviour that we want.
Thanks, LGTM.

Nice one, thanks, LGTM.

Little friendly ping: how do we like this little reshuffle?

Adding to my previous comment:

Here's my "pen-and-paper exercise" of the problem that we are solving here, and lets start with some examples.
In these examples, empty blocks means there are no instructions in it that are relevant to this problem.

I have upload WIP patch D93694 to show the direction and motivation for this change here.

Thanks for getting to the bottom of this. Agreed, and also LGTM.

Fixed formatting and description of the command line option. Related to this, and in addition to my previous message, this is off-by-default and is currently covered by exactly 1 test:
test/CodeGen/X86/sink-cheap-instructions.ll. After this NFC change, I will look into extending this.

See updated Title and Summary.

Looks like a good fix to me.

LGTM, perhaps wait a day with committing in case there are more comments.

Nice one, LGTM.

Yep, thanks, that should have been const.

Yeah, so thanks again for looking at this. After looking more into this, I don't think this change at the moment makes much sense.

Cheers, LGTM

LGTM

Hmmmm, good points, will check. I was assuming that the legality checks were performed, but yeah, this is impressively broken if not!

Now with a MIR test.

LGTM too.

LGTM

Perhaps some tests are missing: one inlined suggestion is related to AssumeITCMBankConflict, and I was wondering about other cases like Size > 4 and SP relative access?

In D91358#2442675, @bjope wrote:

@SjoerdMeijer : I've reviewed the non-target-specific changes now. And that part looks good to me now.

I don't know if the target-specific parts already has been reviewed (or if anything has changed lately). Can someone with ARM/Thumb2 knowledge can acknowledge that part and set "accept revision".

In D92553#2442344, @RKSimon wrote:

LGTM (with one minor) but an ARM guru should really approve it.

Looking good in general. I think we would need some perf numbers to see if we haven't missed something.
Some more remarks inlined.

Looks like a good change to me, and this looked reasonable:

Cheers, looks like a good fix to me.

Thanks for the feedback.

LGTM

The short story is that this was "accidentally" getting things right.

Yep, LGTM too

I have some high-level questions:

• Are we fighting another optimisations here, some sort of loop-rotate or is this just MBP reshulffling blocks in a way that is not good for us?
• I know we don't nest WLSTPs for profitability reasons, but in theory we could. Not sure we need to check this though. But in general my impression is that some more test can be added, but perhaps you were still working on that.
• I am wondering if some cost-modeling is required. For example, if the iteration count is very low, would that change things?

LGTM

Thanks, and I will look into InsertValue too. That may take a few days, but will certainly do that.

Decided to use TTI.getInstructionCost because getVectorInstrCost doesn't work here; it's an aggregate type, not really a vector.
That means the cost for a ExtractValue will be Free now, which I think is actually more accurate (than 1, and also the original 4).
I haven't added InsertValue yet, because that is e.g. not supported by getInstructionCost as it seems a little bit of a different beast.

Thanks. While I do have plans to look into this more soon, I think it's time to commit this thing as it has been proven to do some good, so will do that a bit later today.