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?

LGTM. I'll commit it for you.

Ping. Does anyone think this is a good idea? Bad idea? Have any further comments?

Add tests for some other types as well.

Other than a few minor nits that can be addressed on the commit, LGTM.

Fixed the typos, added a check for no register move.

Added a check for wrapping in the computation of the new shift amount and mask start bit.

Fixed as part of the original patch. Abandoning this.

Interestingly enough, I tried this (or some variant of this) and it didn't work for me. Clearly I had done something wrong when trying it. In any case, I've run this against the code we were originally spinning on and it's all good. LGTM and thanks for fixing so quickly.

In rL359398#646009, @spatel wrote:

In rL359398#645998, @nemanjai wrote:

Since I messed up the patch I posted, I've decided to just post it for review as a separate patch: https://reviews.llvm.org/D61384
Can you please review it there and we can probably get this fixed up quickly. I'll address any comments you have right away.

Oops, too late. :)
I reverted at rL359695, so let me reopen the original review and merge your patch into that.

Since I messed up the patch I posted, I've decided to just post it for review as a separate patch: https://reviews.llvm.org/D61384
Can you please review it there and we can probably get this fixed up quickly. I'll address any comments you have right away.

In rL359398#645977, @spatel wrote:

In rL359398#645967, @nemanjai wrote:

Could you either pull this patch or advise on the correct way forward (i.e. should I just commit the patch? post it for review?)?

Sorry about that! I'm seeing different crashing on existing regression tests after applying your patch, but didn't step through yet. Any ideas about that?
I will revert this so we don't have the bug while we sort it out.

This causes the old combine/uncombine infinite loop with the following test case compiled for PPC:

$ cat infiniteloop.ll 
define dso_local void @test(float %a, <4 x float>* nocapture %b) {
entry:
  %call = tail call fast float @test2(float %a) #2
  %vecinit = insertelement <4 x float> undef, float %call, i32 0
  %vecinit3 = shufflevector <4 x float> %vecinit, <4 x float> undef, <4 x i32> zeroinitializer
  %0 = load <4 x float>, <4 x float>* %b, align 16
  %1 = fmul fast <4 x float> %0, <float 1.000000e+00, float 1.000000e+00, float 1.000000e+00, float 0x3FF028F5C0000000>
  %mul = fdiv fast <4 x float> %1, %vecinit3
  store <4 x float> %mul, <4 x float>* %b, align 16
  ret void
}

Sorry, could you please rebase to latest? It does not apply cleanly for me (assembly.h does not apply).

I like the patch now and the test case seems to cover what it needs to (I like the indirect call to ensure a call through a global entry point trick).

LGTM.

Please reduce the test case only to cases that are changing behaviour. Also, it is probably useful to commit the test case initially as an NFC patch so we can see the actual difference in codegen.

@pjeeva01 Can you address Jinsong's comments? It would be good to add the negative tests for sure:

• No vectorization with -mattr=-altivec
• No vectorization with the nobuiltin attribute
• A couple of calls to functions that MASSV doesn't have a vector equivalent for

Also, it would be good to add a run line for big endian.

The logic LGTM. Please address the test case updates requested by Jinsong. I don't think this requires another revision though.

Do you need to build clangd? We explicitly don't aim to support building everywhere clang can be built, maybe we should just disable in this case?

Our environment includes various OS levels running on PowerPC. We certainly wouldn't want to disable building/testing clangd on all our PowerPC machines. Is there a way to disable it only on certain OS levels?

Cleaned up the comments and the MB parameter for RLDIC does not need to include the shift that was originally on the RLDICL.

LGTM. Thanks for pulling out this change into a separate patch. I'll commit it for you on Monday. This way others have an extra opportunity to add any comments or concerns.

In D58497#1449306, @dblaikie wrote:

In D58497#1449243, @nemanjai wrote:

Ping.

Unfortunately Richard Smith is out for a few weeks at the moment, so might take a little bit before he can get to this.

It's odd to me that this lacks a test case - but you mention it's shown up on buildbots? Does it reproduce consistently there? Under what conditions (which buildbots/configurations show this - are they permanently failing because of this?)?

A test case, if at all possible, would be super helpful.

The failure this causes always shows up in the Modules/builtins.m test (at least in my experience). It is far from predictable and it does not consistently reproduce on any build bot. It occasionally shows up and slight perturbations in the source make it go away.
Honestly, I don't find this to be all that surprising. Using memory after freeing it has inherently unpredictable behaviour. There are certain toolchains that will diagnose freeing the same memory twice, but that's not the case here - we just happen to use it after freeing it.

Is this patch dead? Does @kbarton or perhaps @jonpa have any interest in seeing this through so IPRA can be enabled on PPC/SystemZ?

@kbarton Do you plan to rebase and commit this patch?

Do you plan to follow-up on these questions and comments?
At least the full context is needed and for the test case, I imagine it can be similar to other driver test cases. I imagine tools/clang/test/Driver/linux-ld.c can be augmented with this?

This looks right. Thanks.

If there are no objections in the next week or so, I'll commit this and it can be reviewed post-commit.

@stefanp Victor doesn't have commit access yet. Can you commit this for him?

I don't really know much about how libunwind is tested so I can't really comment on how to write one (or the need for one). But the code looks good to me.

Can you add a test case? At least for the spills that you're adding.

I am only requesting changes for the refactoring of the function. As I mentioned in the inline comment, I am not necessarily requesting that you rework the addressing mode computation functions as I mentioned, just that you clean up the code in SelectAddressRegReg().

LGTM. The remaining comments are stylistic nits that can be addressed on the commit and do not require another round of review. Thank you for your patience with this review.

Forgot to select Accept Revision from the pulldown.

In D59633#1446706, @jsji wrote:

Might miss some opportunity related to parameters instead. Please have a look. Thanks.

Updated to not look through copies if that leads to a physical register since that will never produce a defining MI.

Also, is it possible to have a test case for this? I don't actually know what libunwind test cases look like, but seems pertinent.

It would appear that the fix for PR 41149 should be fairly straightforward. The vector type legalizer just needs to learn about ISD::ABS. Likely just another case to be added in ScalarizeVectorResult().
PR 41160 is a common pitfall with DAG combines. I've gone ahead and reduced the test case provided in the PR.

I think approaching the problem this way is unnecessarily limited - and I say that knowing that I may have suggested a similar approach.
However, what we are actually looking to do is to convert a DAG such as:

Thanks Teresa, I'll commit this soon.

OK, it appears that the PPC issue is really just a phase ordering problem that is quite easy to resolve. We now get the ISD::ABS node earlier so we may very well end up in a situation where we need to legalize the result type of such node. So our Custom legalization of all vector types can't really work any longer. Applying this in PPCISelLowering.cpp should fix the UNREACHABLE failure:

I will apply this patch and take a look at what breaks on PPC. I'll advise soon on what is needed for us.

Sorry about the delay. LGTM.

LGTM. Sorry about the delay.

In D47332#1424366, @RKSimon wrote:

Is anything happening with this? We've hit PPC issues with ISD::ABS on https://reviews.llvm.org/D49837 and I noticed that PPCTargetLowering::LowerABS has a reference to this patch.

I think we should re-think this patch as it seems to massively increase the number of failed attempts the instruction selector makes when selecting code. Below is a trivial example:

@sfertile Do you have any comments regarding this as you're most familiar with this part of the PPC backend?