LGTM other than a few minor nits.

Oops. Forgot to accept.

LGTM other than the code here looks really messy - but it looks messy regardless of this patch.

Clean up some of the comments, function naming, single-use check and other comments from Jinsong. Thanks for the review Jinsong.

@jsji @amyk Do you guys have any further comments regarding this?

A few minor nits and a functional problem that needs to be addressed (exiting if the input is not a v4f32).

Missed some code cleanup on the last upload.

Updated to handle the load->shuffle pattern in addition to the load->build_vector pattern.

In D63624#1559130, @amyk wrote:

Thanks @jsji for letting me know, and thanks @nemanjai for the handling of the load and splats!

I think this mostly looks good to me. I'm curious though, we also have test/CodeGen/PowerPC/load-v4i8-improved.ll that has a load->shift/permute->splat case. Could this patch be extended to cover this?

Remove unnecessary function and fix up conditions for the transformation.

This breaks test cases on a few targets. Please refer to the original review for details.

Same issue happens with arm64, aarch64, nvptx triples.

This breaks (at least) PowerPC with the typical DAG Combine cycle (i.e. one combine undoes the other in a cycle). Here's a minimal test case to show this:

define dso_local <4 x double> @sub(double %b, double* nocapture readonly %ptr) local_unnamed_addr {
entry:
  %arrayidx = getelementptr inbounds double, double* %ptr, i64 45320
  %0 = load double, double* %arrayidx, align 4
  %vecinit = insertelement <4 x double> undef, double %0, i32 0
  %arrayidx1 = getelementptr inbounds double, double* %ptr, i64 176
  %1 = load double, double* %arrayidx1, align 4
  %vecinit2 = insertelement <4 x double> %vecinit, double %1, i32 1
  %arrayidx3 = getelementptr inbounds double, double* %ptr, i64 2734
  %2 = load double, double* %arrayidx3, align 4
  %vecinit4 = insertelement <4 x double> %vecinit2, double %2, i32 2
  %arrayidx5 = getelementptr inbounds double, double* %ptr, i64 7
  %3 = load double, double* %arrayidx5, align 4
  %vecinit6 = insertelement <4 x double> %vecinit4, double %3, i32 3
  %splat.splatinsert = insertelement <4 x double> undef, double %b, i32 0
  %splat.splat = shufflevector <4 x double> %splat.splatinsert, <4 x double> undef, <4 x i32> zeroinitializer
  %div = fdiv fast <4 x double> %vecinit6, %splat.splat
  %sub = fsub fast <4 x double> <double 0.000000e+00, double 0.000000e+00, double 0.000000e+00, double 0.000000e+00>, %div
  ret <4 x double> %sub
}

Compile with llc -mtriple=powerpc64le-unknown-unknown

Added a couple more reviewers that either recently modified this file or I think may be interested in the proposed change.

Remove the double cast. Simplify the test case. Rename the temp.

Also, please make sure that the summary and text of the commit message does not mention PGO since it is not really considered any longer.

Removed redundant pattern.
Added commuted tests, a v2i64 test and a negative (v4i1) test.

I'll commit this for you.

LGTM. I assume you're only posting this for review because you don't have commit access.

The LLVM portion of this patch committed in https://reviews.llvm.org/rL362568.

I'll commit this for you soon Jeeva.

This was identified as the culprit commit that broke the PPC big endian bot: http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/27963

LGTM.

Okay, but we just call MBB->getParent()->getFunction().hasProfileData(), where do we actually check that the loop is hot?

Also, even if we align the majority of loops, how much does that really cost us? The code-size impact could be minor compared to the perf improvement, and if so, we should just always do it. It is still true that most users don't use PGO.

Please name the test case csr-save-restore-order.ll rather than using the word reverse since the reversal is something you do once and the test case shows the order of saves/restores.

LGTM. Thanks for getting this done.

Can you explain why we don't always do this? You're checking for profiling data but then not using it?

It would be interesting to see how that result translates on a more typical x86 build machine. Either way, I suspect we'll get different opinions about whether a 0.3% time increase is minimal and whether that cost is worth paying for the runtime perf gains. This might be a case for differentiating between -O2 and -O3?

Remove an unrelated change that snuck in by accident.

Update the new test case. Thanks @efriedma for the tip.

In D61726#1498006, @thakis wrote:

Looks like the new test is failing: http://lab.llvm.org:8011/builders/clang-cmake-x86_64-avx2-linux/builds/9439/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Areassociate-after-unroll.ll

Move the newly added test case and update it to only show the different behaviour (after committing it to show the current behaviour in r360426)
Move the additional run of reassociation before the late LICM pass. I assumed that this is a good place for it in the pipeline since LICM might move things out of the loop and potentially take away some opportunities. This is just based on a weak hunch and I am very much open to suggestions for a better place for this in the pipeline.

In D61726#1497009, @efriedma wrote:

Running reassociate after unroll probably makes sense. But I'd like to see compile-time numbers.

How carefully have you considered the exact placement? It looks like you're using different placement for the legacy vs. new pass manager. Do we want to run before the late LICM pass?