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.

Also, I've thought a bit about what Jinsong mentioned and I am wondering if maybe we don't want to take a different approach. Namely, the cost model is meant to return the cost of individual instructions with a given type and this cost is the instructions latency relative to the latency of a simple scalar ALU instruction (I think a comment somewhere uses add as the basis for what 1 means). And the vectorizers use these values to essentially add up the costs of all the instructions in a loop/block and then compute what provides the lowest total cost assuming that the cost of an N-wide vectorization can be computed by simply dividing the total cost by N.

I think this code could be greatly simplified if we implement int PPCTTIImpl::vectorCostAdjustmentFactor(Type *Ty) which would return 1 by default and 2 if we are on a subtarget with vector operations that take up two execution units and Ty is a vector type.

The remaining comments from me are minor. I am OK with this going in if Robert is.

In D55503#1332735, @sarveshtamba wrote:

Hello @nemanjai @compnerd, please let me know how to proceed on this further.

In D55503#1329617, @sarveshtamba wrote:

The Swift 5.0 toolchain on PowerPC64LE also works fine if we remove the "defined(ppc)" check altogether in the original code i.e keep only "!(defined(NetBSD) || defined(OpenBSD)))".
Would that affect other PPC platforms (32/64 and BE/LE PPC other than PowerPC64LE)?

Yeah, as we discussed over IRC, all I was suggesting is that there is a difference when we choose one over the other with this patch. But I don't think we really care about endianness here, so we might want to just change this to defined(__ppc__) || defined(__PPC__) so we always define LLVM_THREADING_USE_STD_CALL_ONCE the same way on all PPC platforms.

Since we have discovered this in the PPC back end but the issue also exists in ARM and X86, it'd be nice to notify @t.p.northover (already a reviewer) and @craig.topper about this potentially needing to change in their back ends as well.

This code has been in place for quite a long time as there is some sort of bug in libstdc++. I don't remember what the problem was but I remember that this was required because of an issue with the GNU C++ runtime.
My concern is that the intent with defining LLVM_THREADING_USE_STD_CALL_ONCE to zero when __ppc__ is defined is that we would use the non GNU one on all PPC platforms.
However, gcc does not define __ppc__.
So the behaviour before this change is that anything built with clang on any PPC system will use the clang version of std::call_once and anything built with gcc will use the GNU version. This was the case on all little endian and big endian systems (both 32 and 64 bit). After this change, this is what the situation will be (compiler-rt == clang's libc++, GNU == GNU libstdc+++):

:) It seems these have been around for a long time and nobody bothered to fix it. Thanks for fixing this.

I am definitely on board with this refactoring since it makes the purpose of the various pseudo instructions more clear.
However, I really don't like the name PhonyInst. These will lead to real instructions being emitted during emit time. I think we should name these according to when we expect to expand these (similarly to what you did for the custom inserter ones).
The names should reflect the uses:

• Custom inserter
• Post-RA expansion
• Emit-time expansion
• Asm parser expansion (PPCAsmPseudo)

I suppose since it is in the ABI, it is fine to leave the magic numbers there. And thanks for adding the comment. However, please verify that this mapping applies for ELF v1 as well and if it doesn't please add an appropriate guard.

I'll let @renenkel approve or request changes to this as he understands the math much better than I do, but I do want an investigation into how the INFINITY/QNAN tests work along with a comment explaining that. Particularly since I would have expected that every __int128_t value is a number (i.e. not a NAN) and none of them produce an infinity.

Let's add the #include for the file that defines the pertinent macros and push this change so we start running the test cases on PPC64LE.

In D54824#1306039, @steven.zhang wrote:

Personally, I don't like writing the test case using such script. Because, it will check all the instructions and make your test point unclear. And it hard code the register name, which make this test fragile。

Please let me know if there are any further comments from anyone. Otherwise, we can probably proceed with this.

The PPC changes seem to be neutral, so no objection from me. Would you mind adding a comment to https://reviews.llvm.org/D54663 if this gets committed before that one as I'll have to update the patch? I'll do the same if I commit mine first.

LGTM.

LGTM.

Feel free to just commit patches like this. It appears you're just adding a test case ahead of a patch to make the review easier as it shows only the diffs in code gen.

The "recent patch" mentioned in the description is https://reviews.llvm.org/D53346.

Actually, the handling for v2i32 is already optimal due to the DAG combine that handles this. We'll leave it in the DAG combine since that handles extractions even if they didn't come from legalizing v2i32 types (i.e. it handles a superset of the legalization added here).

Removed the actual definitions of the static arrays from the header. Simply provided the macro for the defs in the header and the macro is used in only the two CU's that need it.
This way if we need to add any new ones, they just need to be added to the macro.

LGTM now that we don't bitcast types any longer.

In D54449#1296245, @dschuff wrote:

Conceptually, COMPILER_RT_ABI is the calling convention used by all compiler-rt functions. It is explicitly named and used because it may be different from the default calling convention on some targets. In practice I believe it only affects ARM, where the calling convention uses ARM's soft-float ABI (where IIRC floats are passed in integer registers) even on targets that use the hard-float ABI by default. The compiler knows the calling conventions and emits the right code when it emits the libcalls, and the ABI declaration on the implementations causes the right calling convention to be used there. The unusual thing about the tests is that they locally declare the compiler-rt functions explicitly. So this declaration has to match the implementation. It may not matter for PPC but my recommendation would be "just use it everywhere in compiler-rt" for simplicity.

This is how I meant to do it initially but was reluctant to introduce a new header dependency. However, I don't think it's an issue at all for PPCMCTargetDesc.h to have a dependency on MC/MCRegisterInfo.h.

Did the same for the disassembler.

@hubert.reinterpretcast Have your comments been addressed adequately in the latest version of the patch? Do you have an opinion on adding the test case I proposed?

Just for clarification (and please add the text to the commit message), this is actually required by the ABI:

Each element of the result vector is the result of logically right shifting the corresponding
element of ARG1 by the number of bits specified by the value of the corresponding
element of ARG2, modulo the number of bits in the element. The bits that are shifted out
are replaced by zeros.

The test case fix can be done on the commit. Approving this now.

Since this fixes the sanitizers on PPC for the newer kernels and the fix seems perfectly reasonable and there are no concerns brought up by anyone, let's go ahead with this fix.

Please revert this. The way this was committed is incorrect. You've created directories such as llvm/trunk/llvm/ which we do not want. I suppose this has something to do with the fact that the patch was created using the monorepo and committed in some different way.

LGTM. Thanks.

Since the logic in the switch statement is difficult to follow, I ended up having to test this patch with a script that tries all combinations of comparisons, results and operand order in order to convince myself that it doesn't emit a setb when it is not valid to do so. Everything checks out there, but I don't really have an easy way of testing whether any opportunities are missed.
I am really hoping that you can simplify the logic in that switch to make it easier to follow.

I think the changes that are needed are clear enough that this doesn't require another review cycle. Approving this with the assumption that the required change will be made on the commit.

LGTM.

Updated the regular expression to just eat any text that is not relevant between the function directive and its text. This seems to work for both ELFv1 and ELFv2 asm output.

Implemented all the patterns for codegen of extract->store.

Modified the changeset to not reduce the capabilities of the pass (select the correct opcode based on the register for post-ra).