Hi Dave. I have a quick question. Have you considered tuning the current cost model? What are the compile time implications of using VPlans?

@craig.topper Good catch. Thank you. I've updated the diff.

@wenlei You are right. SmallVector is a better fit. I updated the patch.

@modocache Thank you for the review. I updated the permissions of the diff. Sorry, for the trouble.

Thank you. I will make the changes and try to commit the patch.

LGTM!

LGTM.

LGTM!

Ayal, I commented on the bug report that I don't understand why this heuristic is useful in the general case (for loops that are not this specific loop). Are you seeing any speedups on SPEC or the LLVM test suite (or other test suites?).

Magnus, I don't have any problem with this change because it is specific to Erlang. I suggest CC-ing the original author of adjustForHiPEPrologue, and if that person has no objections commit the change. Relying on metadata is not idea, but I understand the constraint.

LGTM.

Hal, the code looks much better. I was thinking about this patch on the way to work this morning and I was wondering if you could mark the type <4 x i32> as legal, because you _can_ load it and store it to memory. Right?

I don't like the approach of passing in address-of-bool as parameter argument, especially since you did not document the parameter (is it IN, is it OUT, etc).

Hal, I am not sure I understand the problem. Is the problem register pressure or the fact that store <8 x i32> is more expensive than 8 times store i32?

LGTM.

The patch LGTM.

Sorry for the delay in review. This patch LGTM.

Sounds good. Thanks Matthew.

Did you see any performance improvements due to this commit? Did you measure compile times?

LGTM.

LGTM.

Elena, please fix the cost table and don't change anything else.

We don't let the instruction Sitofp <4 x i32 > %a to <4 x f64> go through the type legalizer.

Elena, I don't understand your comment. getTypeLegalizationCost imitates the type legalizer by splitting and promoting. It actually uses the type legalizer. What do you mean it does not support zero-extend and sign-extend?

Thanks for explaining this Elena. Have you considered handling all of the special cases by adding them to the 'TypeConversionCostTblEntry' table? Also, have you considered improving getTypeLegalizationCost?

LGTM Amjad!

Thanks for fixing this Charlie!

It looks like this patch is not ready for review.

@anemet In SelectionDAG we can use the memory order chains, and don't have to scan the whole basic block for loads/stores. This is very efficient, and is already used by the load-store merger in SelectionDAG.

In D14829#307505, @mssimpso wrote:

Hal,

Thanks very much for the follow-up. Yes, I'm working on compile-time along with the type shrinking work I previously mentioned. I will post an updated revision soon.

LGTM.

Matthew, thanks for working on this.

Yes, we should merge this. LGTM!

I did not get a chance to review this patch carefully. Andrea, Simon, David, Elena, Sanjay, did you get a chance to review the patch? Does it look okay? I did not see a LGTM in the thread.

LGTM.

Did you measure the performance impact of this patch on the llvm test suite (or SPEC or other test suite?). Is this a win?

LGTM.

LGTM.

LGTM.

LGTM!

LGTM. Please commit.

LGTM!

I don’t see a problem with this commit but please email llvm-commit and allow other developers to review this patch.

Okay. Please go ahead and commit this patch. I do have one minor comment:

I am okay with this change, but I have a few comments on the patch:

What about SLP-vectorization of stores? I suspect that we have the same bug for stores.

In D9804#252514, @hfinkel wrote:

At a high level, this transformation seems overly restrictive, and will need cost-modeling work. A couple of thoughts:

1. I don't see why you're restricting this to extracts used by stores (or inserts fed by loads); if the goal is to save on [zs]ext instructions, then this seems profitable regardless of how these are used. Moreover, I don't understand why there's a hasOneUse() check on the [zs]ext instructions.
2. The [zs]ext instructions that you're trying to eliminate might be free, at least in combination with the extract or insert, rendering this a bad idea. Consider the (unfortunately common) case where the target does not actually support a vector extract at all, and so it is lowered by storing the vector on the stack and then doing a scalar load of the requested element. In this case, if the target supports the corresponding scalar extending load, the extension is free. Likewise, for those [zs]ext fed by loads, these might be free if the target supports the corresponding extending load. Worse, the vector [zs]ext you're forming might not be legal at all (this is the most-serious potential problem).

I think that this is a good change. In many other places in the vectorizer the design was that we let other passes (such as CSE, InstCombine and LSR) clean up after us. I am totally okay with letting LSR do the cleanup. If I remember correctly we have always relied on LSR to do the cleanup and I don't remember why we have the logic for searching other induction variables.

Maybe in a future patch we should refactor this logic in the SLP vectorizer and LoopVectorizer into a single function that verifies the metadata kind? I assume that the list should be identical.

The code LGTM. Did you measure the compile time impact of this change?

Sanjay, this patch looks okay. I think that the compile time hit should be minimal but I think that we need to measure just to be sure.

LGTM!

Thanks for working on this. Did you run the llvm test suite? Are there any performance wins or compile time regressions?