Requesting changes because there is no BE support.

Due to an issue with the machine used for this commit losing connection during commit, only part of the patch was committed. We're working on getting the rest of it committed to bring the bots back to green.

Some minor changes are required to make it more clear what this does.

Other than the minor nits that you can feel free to fix on the commit, LGTM.

It would appear that we are converging here. Both @jsji and @spatel are presumably in the "Must Review" for this. Would you guys mind taking another look as your approval is necessary for this to proceed.

Forgot to select Accept.

LGTM.
If we are producing the checks with the script, we should refrain from manually changing them as any future updates to the test will make it very difficult to determine whether the change is intended or not.

LGTM. I'll let Stefan give the final ack though.

I have not seen this problem resurface to be honest. When we initially hit it, changing the path for the build worked around the problem for us so we weren't really hitting in any longer. I posted this because I realized the possibility exists of having these dangling pointers (and it certainly fixed our original problem even without changing the build path).
However, I must say that I am completely out of my depth here as I am not a front end developer and don't really know how any of this stuff is supposed to work (i.e. whether it makes sense for PP here to be pointing to something that will go away).

LGTM other than a couple of minor nits. Thanks for cleaning this up.

I suppose this is essentially an RFC to gauge the community's interest in improving the algorithm to gain more precision.
I think that in order to pull the trigger on a change such as this we would need the following to happen:

1. The patch needs full context to be reviewable
2. The formatting needs to be fixed up (some lines too long, etc.)
3. We need testing (I imagine this makes a bunch of LIT tests fail which need to be updated)
4. b) It might not be a bad idea to add a test to test-suite that will do some fast division vs. non-fast division to make sure the accuracy isn't too bad
5. We should do some thorough analysis of the accuracy of the algorithm vs. the HW implementation on a wide range of values on a couple of targets (as was suggested above). I know this was done on PPC, so please share those results and try to replicate on another easily available target (such as X86).

Please note that essentially all the code in the SelectAddress* functions is somewhat orthogonal to this. Those changes are required to optimize what this patch does, but if the reviewers prefer, I can pull those out into a separate patch. However, I opted to keep them part of this patch at least initially as motivation for those changes on their own would be hard to understand without the context of this patch.

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.