LGTM.

LGTM.

LGTM with a simplification to the test case.

LGTM.

I am sorry, my suggestion turned out to be wrong. I have suggested the fix in the original bug (and please add the IR for the motivating test case).

Thanks for refactoring this. Please add [NFC] to the start of the summary.

In D100290#2799987, @tstellar wrote:

What is the status of this patch? It fixes a blocking bug for the 12.0.1 release.

In D103700#2799340, @reames wrote:

In D103700#2799338, @nemanjai wrote:

We will be happy to test this on both our LE and BE systems. @stefanp do you have time to test this out or should I do it?

Seems like overkill for this one given the change is trivial and the test is target independent.

I'd save that for when I go to reapply the original patch that exposed this.

We will be happy to test this on both our LE and BE systems. @stefanp do you have time to test this out or should I do it?

Please be sure to add reviewers from IBM that are actively developing this area of the PowerPC back end on your patches.

It certainly does sound odd to have a subreg on a def of a virtual register in SSA. We'll see where this is coming from and why but just a quick look at the -print-after-all output reveals that it is the peephole optimizer that produces it. We'll look into whether this is a bug in the peephole optimizer or if this is some PPC callback called from the peephole optimizer that does this.

I think that the original author (Matthias Braun) no longer contributes to LLVM as I haven't seen any activity from him so I'll add some reviewers that may be familiar with this code.

LGTM. Thanks for fixing this.

When recomitting this, please apply the changes from https://reviews.llvm.org/rGcf2eeb114c59 because I will have to pull those to resolve the issue it causes after you pulled this.

Oh, I think this is an ordering issue since ca5f07f8c4bc96d16ed1992b810aa3897df157f2 pulled that enumerator.

I think this is fixing a problem that isn't real. I am on ToT: 0c4f76c9421a9a72a9c86321628abcb31881ae94 and as far as I can tell llvm::TargetStackID::WasmLocal doesn't exist. However, the default label is causing real failures with -Werror with error:

lib/Target/AMDGPU/SIFrameLowering.cpp:665:3: error: default label in switch which covers all enumeration values [-Werror,-Wcovered-switch-default]
  default:
  ^
1 error generated.

Thanks everyone for providing feedback on this. I posted this to gauge interest in the community for such a change. As it appears, the consensus seems to be that this isn't desired so I will abandon this change and vendors will continue with their homegrown solutions to this problem.

LGTM. Thanks.

LGTM. Thanks for implementing this. The redundant pseudo can be removed when committing.

In D98261#2704839, @arichardson wrote:

Thanks, I wasn't sure about PPC64. If I understand correctly, the solution for this would be to change the #if __LDBL_MANT_DIG__ == 113 check to sizeof(long double) > 8. And instead of using a f128 typedef there needs to be a f128_tf and a f128_kf type (or some better name)?

Can you please add a description to this patch describing what this is meant to acomplish, what PPC 32-bit platforms you plan to enable this on. Also, whether this is expected to increase build/test times on the PPC big endian bots, etc.

Can you please add a description for this review. Why this is needed, why only for PPC32 and not PPC64, who the intended users are, etc.

I don't really see any issues with this but I think that @chmeee or @joerg would certainly be more qualified to approve this.

Although I am obviously not opposed to adding these instructions and the necessary register classes, we do have to devise a way to ensure RA doesn't allocate the same register to the base and result of LQ. An additional complication for this check is ensuring that if RA == 0, RTp != 0.

LGTM. Of course, please ensure that this fixes the issue when the dependent patch is applied prior to committing this.

In terms of the motivation for this, the description of the patch should include something along the lines of:

LGTM. Thanks.

LGTM. Thanks.

Please add some info about how this affects functions that need the frame pointer. Do we get more/less saves/restores? Does something go wrong?

LGTM.

LGTM other than the allocatable nit. Let's go with this fix for now to unblock the kernel build and we'll follow-up with a more complete fix for other reserved registers later.

Looks like this is causing failures at https://lab.llvm.org/buildbot/#/builders/76/builds/2422
Please revert.

I thought I had selected "Accept" when adding the last comment.

LGTM.

This seems fine to me now but I'll defer to front end experts for approval.

LGTM other than the missing assert. Also, please look into the interaction between prefixed instructions, pc-relative memops, paired memops and whether we do the right thing when 1/2 of those are turned on and others turned off (this can be done in a subsequent patch).

LGTM other than the nit that can be addressed on the commit.

LGTM (as long as the nits are addressed).

Please update regsiters in the title to registers.

LGTM. Maybe give this a couple of days to see if any other reviewers have further input.

LGTM.

LGTM.

LGTM. This turned out very nice.

LGTM.

Please add the missing BE handling.

LGTM. Thanks for fixing this.

In D101342#2735929, @jrtc27 wrote:

In D101342#2735912, @RKSimon wrote:

LGTM - please can you raise a bug detailing the PPC poor codegen for i32/i64 promotions

Filed as https://bugs.llvm.org/show_bug.cgi?id=50217. Does this need a review from a WASM person given the changes there?

LGTM.

In the previous revision I suggested the following test case:

#include <stdio.h>
typedef int __attribute__((vector_size(64))) v8si;
static void __attribute__((noinline)) printWideVec(v8si *ptr) {
  v8si WideVec = *ptr;
  printf("Vector: { ");
  for (int i = 0; i < 7; i++)
    printf("%d, ", WideVec[i]);
  printf("%d }\n", WideVec[7]);
}

Changed rsqrt to be an actual reciprocal rather than just a refined sqrt estimate.

Also, this is another one we should merge to 12.0.1 so please open the bug, mark it as a blocker and link it here.

This will turn off argument promotion for all types wider than 128 bits. An example of a test case that might suffer from worse codegen:

#include <stdio.h>
typedef int __attribute__((vector_size(64))) v8si;
static void __attribute__((noinline)) printWideVec(v8si *ptr) {
  v8si WideVec = *ptr;
  printf("Vector: { ");
  for (int i = 0; i < 7; i++)
    printf("%d, ", WideVec[i]);
  printf("%d }\n", WideVec[7]);
}

There may be something I am overlooking here, but I really don't think we need to or want to change the back end. Just add the new builtins to the front end as aliases to the existing ones and generate the same code from the front end.

The fixes in https://reviews.llvm.org/D101214 are sufficient to bring the -Werror bootstrap back to green. Please review the patch.

In D79714#2713610, @xbolva00 wrote:

I pushed a fix to just disable this warning for googlemock/gtest.

The rest of the comments I have are minor comment and code restructuring nits. Those can be addressed on the commit. Otherwise, LGTM.

This breaks Clang's ability to bootstrap LLVM with -Werror as evidenced by the buildbot failure https://lab.llvm.org/buildbot/#/builders/36/builds/7587.
Please revert or fix the googletest code that trips this warning. Pertinent file: