In D147336#4237118, @dmgreen wrote:

Hello. About the performance changes - I have been looking into them a little. A lot of it looks good, especially where gather/scatter are available which this can make use of. There are some cases where it wasn't doing quite as well under some places in MVE, but https://reviews.llvm.org/D147331 should hopefully sort out the largest of them. Sam/Nick should be back on Monday to review it. We have some other cases where gather/scatter are not available for whatever reason and those don't look as good but the changes are much smaller. Again it likely comes down to adjusting the costmodel, perhaps for the cost of the scalarized loads in getMemInstScalarizationCost needs to be a little higher under MVE. I wouldn't consider any of that blockers considering all the improvements.

This is all good news, thanks!

@dmgreen I'd really appreciate any input you can share on the performance swing you observed.

In rG498aa534f472d28db893aa9a8627d0b46e17f312#1189878, @dmgreen wrote:

Hello - We also saw some of the similar assert above, along with quite a few very large performance changes. Some of those were good but some of them looked pretty bad. I've reverted until the correctness issues can be resolved and am trying to look through the performance - it might well be that the cost model isn't expecting non-constant pointer inductions, so is not getting the correct costs.

I've posted a review with a second attempt here: https://reviews.llvm.org/D147336. This contains the functional fix, but not yet anything on the performance side. Let's continue the discussion on the perf issue there.

Use wording suggested by @asb

Two levels of comments here.

LGTM

LGTM

LGTM once Craig's style comment addressed.

LGTM w/comment

LGTM

Looking at some code which benefits from this change, I noticed an opportunity for a follow up.

LGTM w/minor comments.

LGTM - once all the earlier patches have been approved.

LGTM - though can you split this into two patches when you land? The SLP bit deserves a slightly different description focus.

LGTM

I think you're going one step too far with this patch. Not in the sense that this patch isn't justified - only in the sense you should do a simpler case first.

Address review comment

Address review comment.

p.s. The previous case might be interesting to explore later. It should involve a stride store, but separating that into it's own patch series makes complete sense.

LGTM

Ok, at this point all the known soundness problems are fixed in tree. I was simply fixing ones obvious on inspection, and have not done any testing of this mechanism beyond the LIT tests themselves.

I landed a fix for this in 00fdd2cb6c55744ae21f18c377854e9359273454. This patch can be abandoned.

LGTM

In D146457#4209092, @nikic wrote:

LGTM

All of the test changes are, to my eye, semantically the same.

I don't think they are: Let's say that %length == 0, which means that the BECount is -1. Previously, this just extended %length to i64, so that the effective trip count was zero. Now we instead extend -1 and then add the stride, making the trip count correct. The previous code was using an incorrect order of extensions.

So, you'd be correct here, except that the IV in question is nsw, and we branch on the result. For N=0, the starting value of the post-inc IV for the loop is -1 and we subtract 1 on every iteration. To reach zero, we have to pass through the negative overflow case, and thus have poison and (from the branch) UB. So, we can discount this case when reasoning about the expansion.

lsr-term-fold is still wildly unsound. I have patches out to address this, but we should wait until that is done discussing enabling it (by default or by target).

Another simplification.

LGTM

LGTM

LGTM