[PowerPC] Combine 64-bit bswap(load) without LDBRX

[nemanjai]

Summary

When targeting CPUs that don't have LDBRX, we end up producing code that is very inefficient and large for this common idiom. This patch just optimizes it two 32-bit LWBRX instructions along with a merge.

This fixes https://bugs.llvm.org/show_bug.cgi?id=49610

[spatel]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

15245

If we honor the clang-tidy (no else after return), that makes it more obvious that we are repeating conditions in both clauses. It would be cleaner to hoist those into a common 'if' clause, and probably better still to make it an early exit for the inverted clause (possibly add a helper function too):

if (!Is64BitBswapOn64BitTarget || !IsSingleUseNonExtLd) return SDValue();

15246–15247

Do we need to check for 'volatile' or other modifiers?
Are we ok with misaligned accesses?
Should add regression tests either way.

llvm/test/CodeGen/PowerPC/ld-bswap64-no-ldbrx.ll

3

It would be better to include tests for the minimal patterns - just a load or just a store. We are not handling the store case yet, so that might be a "TODO" item (or doesn't matter if we don't need to worry about the pre-stdbrx targets):

define void @split_store(i64 %x, i64* %p) {
  %b = call i64 @llvm.bswap.i64(i64 %x)
  store i64 %b, i64* %p, align 8
  ret void
}

define i64 @split_load(i64* %p) {
  %x = load i64, i64* %p, align 8
  %b = call i64 @llvm.bswap.i64(i64 %x)
  ret i64 %b
}

[nemanjai]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

15245

Oh, I probably should have noticed that the if clause has a return. I'll update it. Thanks!

15246–15247

There are no alignment requirements for the byte-reversed loads/stores. I'll add a test.

llvm/test/CodeGen/PowerPC/ld-bswap64-no-ldbrx.ll

3

Yes, the store combine is definitely a TODO :)
I'll update the tests, thank you.

[nemanjai]

Fix up clang-tidy warnings and add some more regression tests.

[spatel]

LangRef says "the backend should never split or merge target-legal volatile load/store instructions":
https://llvm.org/docs/LangRef.html#volatile-memory-accesses

I haven't looked at the use cases in detail, but the target does support 64-bit loads via plain ld, so we shouldn't do the transform?

[nemanjai]

In D104836#2838555, @spatel wrote:

LangRef says "the backend should never split or merge target-legal volatile load/store instructions":
https://llvm.org/docs/LangRef.html#volatile-memory-accesses

I haven't looked at the use cases in detail, but the target does support 64-bit loads via plain ld, so we shouldn't do the transform?

Hmm... clearly this requirement cannot possibly be met for volatile operations that are wider than the available load for the target. Of course, that is not the case here. So I suppose it is possible for a volatile load to load the two halves of the value before and after a store to the same memory from another thread.
I'll add the volatile check and bail on the combine. Thanks for bringing this up.

[nemanjai]

Do not combine volatile loads.

[spatel]

See inline for a couple of small adjustments, otherwise LGTM.
I'm not up-to-speed on current PPC targets though, so might want to wait for a 2nd opinion.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

15275

I forgot to mention atomics (add one more test...).
I think we want to use "LD->isSimple()" to be safe.

llvm/test/CodeGen/PowerPC/ld-bswap64-no-ldbrx.ll

20

IIUC, we are ok doing the transform with any alignment, so this test isn't capturing that.

Please add a misaligned-only test or adjust the first test to have "align 1" so it can verify that we perform the fold independent of alignment.

[nathanchance]

I can confirm that this reduces the stack size of the function in the Linux kernel that prompted the report:

Before:

arch/powerpc/kvm/book3s_hv_nested.c:289:6: warning: stack frame size (2048) exceeds limit (768) in function 'kvmhv_enter_nested_guest' [-Wframe-larger-than]
long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
     ^
1 warning generated.

After:

arch/powerpc/kvm/book3s_hv_nested.c:289:6: warning: stack frame size (1856) exceeds limit (768) in function 'kvmhv_enter_nested_guest' [-Wframe-larger-than]
long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
     ^
1 warning generated.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

if (Subtarget.needsSwapsForVSXMemOps()) {
       default:
         break;
       case Intrinsic::ppc_vsx_stxvw4x:
       case Intrinsic::ppc_vsx_stxvd2x:
         return expandVSXStoreForLE(N, DCI);
       }
     }
     break;
-  case ISD::BSWAP:
+  case ISD::BSWAP: {
     // Turn BSWAP (LOAD) -> lhbrx/lwbrx.
-    if (ISD::isNON_EXTLoad(N->getOperand(0).getNode()) &&
-        N->getOperand(0).hasOneUse() &&
+    // For subtargets without LDBRX, we can still do better than the default
+    // expansion even for 64-bit BSWAP (LOAD).
+    bool is64BitBswapOn64BitTgt =
+        Subtarget.isPPC64() && N->getValueType(0) == MVT::i64;
+    bool isSingleUseNonExtLd = ISD::isNON_EXTLoad(N->getOperand(0).getNode()) &&
+                               N->getOperand(0).hasOneUse();
+    if (isSingleUseNonExtLd &&
         (N->getValueType(0) == MVT::i32 || N->getValueType(0) == MVT::i16 ||
-         (Subtarget.hasLDBRX() && Subtarget.isPPC64() &&
-          N->getValueType(0) == MVT::i64))) {
+         (Subtarget.hasLDBRX() && is64BitBswapOn64BitTgt))) {
       SDValue Load = N->getOperand(0);
       LoadSDNode *LD = cast<LoadSDNode>(Load);
       // Create the byte-swapping load.
       SDValue Ops[] = {
         LD->getChain(),    // Chain
         LD->getBasePtr(),  // Ptr
         DAG.getValueType(N->getValueType(0)) // VT
       };
if (isSingleUseNonExtLd &&
       DCI.CombineTo(N, ResVal);
 
       // Next, combine the load away, we give it a bogus result value but a real
       // chain result.  The result value is dead because the bswap is dead.
       DCI.CombineTo(Load.getNode(), ResVal, BSLoad.getValue(1));
 
       // Return N so it doesn't get rechecked!
       return SDValue(N, 0);
+    } else if (is64BitBswapOn64BitTgt && isSingleUseNonExtLd) {
+      // Convert this to two 32-bit bswap loads and a BUILD_PAIR. Do this only
+      // before legalization so that the BUILD_PAIR is handled correctly.
+      if (!DCI.isBeforeLegalize())
+        return SDValue();
+      LoadSDNode *LD = cast<LoadSDNode>(N->getOperand(0));
+      SDValue BasePtr = LD->getBasePtr();
+      SDValue Lo = DAG.getLoad(MVT::i32, dl, LD->getChain(), BasePtr,
+                               LD->getPointerInfo(), LD->getAlignment());
+      Lo = DAG.getNode(ISD::BSWAP, dl, MVT::i32, Lo);
+      BasePtr = DAG.getNode(ISD::ADD, dl, BasePtr.getValueType(), BasePtr,
+                            DAG.getIntPtrConstant(4, dl));
+      SDValue Hi = DAG.getLoad(MVT::i32, dl, LD->getChain(), BasePtr,
+                               LD->getPointerInfo(), LD->getAlignment());
+      Hi = DAG.getNode(ISD::BSWAP, dl, MVT::i32, Hi);
+      SDValue Res = DAG.getNode(ISD::BUILD_PAIR, dl, MVT::i64, Hi, Lo);
+      SDValue TF = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
+                               Hi.getOperand(0).getValue(1),
+                               Lo.getOperand(0).getValue(1));
+      DAG.ReplaceAllUsesOfValueWith(SDValue(LD, 1), TF);
+      return Res;
     }
     break;
+  }
   case PPCISD::VCMP:
     // If a VCMP_rec node already exists with exactly the same operands as this
     // node, use its result instead of this node (VCMP_rec computes both a CR6
     // and a normal output).
     //
     if (!N->getOperand(0).hasOneUse() &&
         !N->getOperand(1).hasOneUse() &&
         !N->getOperand(2).hasOneUse()) {
 
       // Scan all of the users of the LHS, looking for VCMP_rec's that match.
       SDNode *VCMPrecNode = nullptr;
 
       SDNode *LHSN = N->getOperand(0).getNode();
       for (SDNode::use_iterator UI = LHSN->use_begin(), E = LHSN->use_end();
            UI != E; ++UI)

llvm/test/CodeGen/PowerPC/ld-bswap64-no-ldbrx.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=powerpc64-- -mcpu=pwr5 -verify-machineinstrs | FileCheck %s
+define void @bs(i64* %p) {
+; CHECK-LABEL: bs:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    li 4, 4
+; CHECK-NEXT:    lwbrx 5, 0, 3
+; CHECK-NEXT:    lwbrx 4, 3, 4
+; CHECK-NEXT:    rldimi 4, 5, 32, 0
+; CHECK-NEXT:    std 4, 0(3)
+; CHECK-NEXT:    blr
+  %x = load i64, i64* %p, align 8
+  %b = call i64 @llvm.bswap.i64(i64 %x)
+  store i64 %b, i64* %p, align 8
+  ret void
+}
+declare i64 @llvm.bswap.i64(i64) #2