Hi Björn. Thank you for looking into this bug and reproducing the problem on aarch64. It sounds like you found the underlying issue in checkMergeStoreCandidatesForDependencies. Do you need help with anything?

Ah, I did miss something. I'll rewrite the second check: "Assume ((c +1 ) << y) != 127"

@craig.topper Thank you for the review, and for writing the proof, of course. This is my understanding of the code and of your proof.

@lebedev.ri Okay. I'll add the extra tests, but I'll do it in a separate diff because I've already landed the pre-commit diff that added the extra tests. I also need to look at the optimization that craig pointed out.

@spatel Thank you for the review. I added the pre-commit tests in a new diff (https://reviews.llvm.org/D117338). I've also made the changes to the comment as you suggested. Are you willing to approve the diff or do you want to see the comment and do another round of review?

@craig.topper Ah. I think that the ICMP_UGT transformation is correct, but if you don't mind I prefer to land the two changes separately. I can start a second review for the ICMP_UGT change. Is that okay with you?

@craig.topper thank you for the review. I removed the two predicated that are canonicalized before this method is called (ICMP_SGE and ICMP_UGE). I also used the script update_test_checks.py to update the check lines. I also verified with alive2 that the transformation is correct for ICMP_UGT, but only when the exact flag is present (link: https://alive2.llvm.org/ce/z/qBhxAM).

@lebedev.ri Good catch. Thank you. I verified that when 'exact' is present the transformation is valid for all predicates.

Added the test merge_store_alias that checks that aliasing loads prevent the merging of the store operations.

@hoy Thank you for the review. Yes, the code in TryToAddCandidate and CandidateMatch checks that all of the stores have the same base. I added a second test where the load aliases and prevents the store merging.

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?