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?

Ping.

Sorry for the delay. LGTM.

LGTM with the minor comments addressed on the commit.

In D56286#1409384, @hfinkel wrote:

Is this STW instruction something that instruction scheduling might move? (@nemanjai ?)

Since I haven't seen any further objections to this, I'll commit this later today.

In D49754#1402790, @vit9696 wrote:

This is a series of patches, which I believe should merged altogether. Currently the following patches are relevant:

No, please don't merge them together. It is much more manageable for review when they're separate patches. I realize that this makes it a bit more difficult for the author to keep the dependency ordering straight, but I think preference needs to be given to the "reviewability" of the code.

I think this is very close, but I really think the reloading code needs to be fixed.

Please, no more patches without context. This one was actually easy to review without context and the comments are minor, so I'm fine with these being addressed on the commit.

I can't really review this without context. Could you please re-upload with context?

I think I misunderstood the original problem this is solving. What I thought it was solving is any inline asm or alignment directives that appear prior to the branch or its target (whichever comes last in the function). However, after looking through the code, it appears that the problem is only if the inline asm or alignment directive appears prior to the branch or its target (whichever comes first in the function).
It would probably be good to explain why it isn't a problem if the block that throws off the computation is between the branch and its target.

Also, as we discussed on IRC, please provide full context to make it easier to review.
I am actually OK with this revision as long as the comments are addressed, but I'm just requesting another revision to ensure all the comments have been addressed.

I can't help but feel like this patch adds further complication to an already excessively complicated function. To put things in perspective - this file is currently 282 lines and 210 of those lines are in a single function. Frankly, I think refactoring this into a few conceptual sections implemented in separate functions would go a long way.

There is a long series of comments in this patch and I am not clear at this point on whether this patch breaks anything or it is fine. Could you please Request Changes if this patch is broken or approve if it is fine?

LGTM once the minor nits are addressed.

This can cause relocation overflows:

$ cat b.c 
double b[1LU << 33];
double foo() { return b[(1LU << _SH) - 1] ; }
void setfoo(double d) { b[(1LU << _SH) - 1] = d; }

I'll apply this and try it to see what is changed in the test cases. I'll report back when that's done.

Does anyone have any further comments or objections to this patch? I would like to commit this and close the PR.

Whitney, if you don't have commit access let me know and I can commit this for you. If so, I'll fix the REQUIRES: asserts in the test cases and fix the no-signed-zeros requirement on the commit.

Please add the following test cases:

1. A stack frame that is just a bit larger than required
2. A stack frame with a CSR save that is not a fixed object
3. A stack frame where we have to use scavenging
4. A test case where everything fits and it saves multiple register types (GPR, FPR, VSR)

LGTM. Maybe give it a couple of days in case someone else has an objection.

Changed the option to standard C++ rather than GNU extensions.

As mentioned in a comment, the WASM tests weren't really meant to indicate that WASM supports the type. Removed the changes to the WASM target.

In D57581#1380609, @ilya-biryukov wrote:

...
Do have a comment, though. Any, reason to use -std=gnu++14 and not -std=c++14?
Most (all?) of the tests do not seem to have anything to do with the gnu extensions, so why enable them?

Oh yeah, the existing condition was always true. Thanks for fixing this. LGTM.

This looks to me like the right thing to do but I added Jim Grosbach as a reviewer since he owns the MC layer.

LGTM. I've had a pretty close look at it and am going to approve it. However, I realized that I was the only reviewer on it so I'll ask you to give it a couple of days to give at least @hfinkel and @jsji (but others as well) a chance to have a look if they are interested.

I think this is no longer applicable as we have set this value correctly for the CPU already. Could you close this review if so?

@RolandF Do you need me to commit this for you have commit access now?

Thank you for the updates. I just have a couple of minor nits to add comments which can be done on the commit. LGTM.

In D56175#1356963, @jsji wrote:

LGTM, as long as we add the missing BE test. Thanks for exploiting this, and also great patience while discussion.

Amy, can we get this committed?

Updated the wrong indices for big endian systems. Added comments for magic numbers for indices/shift amounts.

Sarvesh, I've tried this on a PPC64LE machine with one version of GLIBC and found no problems. However, I am reluctant to just go ahead with a change without testing with a few other versions of GLIBC.

Indentation was off in the td file.

Upon closer inspection, this actually almost never fires on PPC so spending any more time on it does not seem useful. Abandoning this patch.

Updated to remove the patterns for the Altivec versions of vector min/max as they have IEEE semantics wrt. handling NaN. A subsequent patch will legalize the _IEEE versions of the nodes for single precision and provide patterns to match them to vmaxfp/vminfp.

LGTM.

In D47332#1330931, @RKSimon wrote:

This covers PR39130 right?

Maybe worth adding the new vec-min-max.ll test file to trunk with current codegen so this patch shows the improved codegen diff?

We have neglected this for a very long time. Just adding a comment to trickle it up to the top of the review queue and I plan to review it very soon.

I think this is close to ready but there are a few comments that have to be addressed. Also, @hfinkel and @inouehrs do you see anything that needs to be handled in addition to what I commented on?

A couple of questions since I am not all that familiar with clang and am certainly not familiar with this unusual SUSE 32-bit situation:

• We seem to be changing the set of aliases here, but what happens if someone actually explicitly specifies --target=powerpc-suse-linux?
• Do we need to change anything about include paths?
• Can you describe the default triple for clang on SUSE 32-bit PPC? Will it be powerpc-suse-linux? powerpc64-suse-linux?
• Will this change not affect 64-bit PPC SUSE? Namely will the default libraries on actual 64-bit PPC SUSE big endian systems now be 32-bit libraries?
• Can you please add a test case and a patch with full context before this patch can go any further?

Other than the minor nit about the test case, LGTM.

LGTM. We definitely want to go ahead with this.

In D33499#1341236, @ken-cunningham-webuse wrote:

@iains - I have interest in resolving this issue, and also a couple of other lingering bugs in the PPC32Darwin ABI realm. If you have your WIP available anywhere, I'd be happy to have a go at bringing it up to current.

I assume that we are fixing this specific instance because we have a test case that breaks if we turn on verification by default. However, I would assume that we actually have a lot of other failures hiding in here which would be uncovered by doing a bootstrap build with -O0. I am not suggesting that we put all the fixes in a single patch, just that we want to do more thorough testing before we turn on MachineFunction verification by default.