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?

Hi Wei,

Hi Lawrence,

Looks good to me assuming that you write the cost model tests, and that you write tests for the ISel changes.

Looks good to me.

Looks good to me.

This looks good to me.

I am sorry for the bug. Thanks for catching and fixing it. One easy way to write a testcase is to rely on the 'overflow' optimizations in instcombine. See the function overflow_mod_mul in Transforms/InstCombine/intrinsics.ll.

David, the code and test case look great. The transformation is not trivial. Do you mind adding a line of comment that explains why -2 is used? Maybe an example input like 00000001 -> 11111101. Also a comment about how rotate can only pull in 1s and how large x values are undefined.

r231140

LGTM.

Olivier, I don’t really understand this heuristics. It looks like you are adding lots of code to handle a very specific case and I am not convinced that this heuristics is useful for the general case.

Hao, this patch LGTM. This is a major change to the vectorizer so please run the llvm test suite and try to identify miscompiles or regressions.

LGTM.

LGTM.

Okay, so if the code is executed then we should try to come up with a testcase that exposes the bug that you found. Can you modify the test cases that broke and try to expose the bug?

Some changes in the patch should go in separately. For example:

+ / Returns true if vector representation of the instruction \p I
+ / requires mask.
+ bool toBuildMaskedVectorInst(const Instruction* I) {
+ return (MaskedOp.count(I) != 0);
+ }
+ void SI();

Everything except for the function WillNotOverflowSignedMul looks great. I also agree with Philip that the refactoring should go in as a separate patch. The first part of WillNotOverflowSignedMul looks correct. The second part (SignBits == BitWidth + 1) is more complex and I prefer that someone else would take a look. I believe that Michael Gottesman is a good candidate for reviewing this part of the code.

Hi Elena,

LGTM! Thank you for the detailed measurements!

This looks like an obvious bug that should be fixed, unless the code is never executed. Can you try to delete this optimization altogether and see if any tests break? You can also try to use llvm-stress to generate lots of code and see if it ever reaches this optimization.

I did not review the code carefully but overall it looks good.

Hi Michael,

Can you please add a check line for ‘ret’ ? I want to make sure that the check line does not mix with code from the following test. With that the code LGTM.

Is there a way to reduce the size of the testcase? Do you need two basic blocks? Can you add additional check lines to make sure that the ‘mov’ that you are matching is the one you are expecting?

Hi Alexey,

I think that he is trying to disable code duplication because GPU languages may contain barriers that must not be duplicated.

The functionality of the patch looks okay. Please add a comment that says that non-negative values override the internal default.

Pavel, I am sorry for the delay in review. I am not that familiar with this code. Can you check with svn-blame and see who are the last few people who touched the code?

LGTM!

LGTM. :)

LGTM! Thanks Sanjay!

Sanjay, you can commit changes like this without waiting for pre-commit code review (See item #3 in http://llvm.org/docs/DeveloperPolicy.html#code-reviews).

LGTM.

In this patch you are changing the instruction V regardless if vectorization succeeds or not. Would it be possible to only change the VL vector without modifying the binary operator V?

Hi Hal,