[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

4

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

4

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

15254

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

19

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 IsSingleUseNormalLd = ISD::isNormalLoad(N->getOperand(0).getNode()) &&
+                               N->getOperand(0).hasOneUse();
+    if (IsSingleUseNormalLd &&
         (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 (IsSingleUseNormalLd &&
       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);
     }
-    break;
+    // 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() || !Is64BitBswapOn64BitTgt ||
+        !IsSingleUseNormalLd)
+      return SDValue();
+    LoadSDNode *LD = cast<LoadSDNode>(N->getOperand(0));
+
+    // Can't split volatile loads.
+    if (LD->isVolatile())
+      return SDValue();
+    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;
+  }
   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()) {

llvm/test/CodeGen/PowerPC/bswap-load-store.ll

 ; PWR7_64-NEXT:    blr
   %tmp1 = getelementptr i8, i8* %ptr, i32 %off
   %tmp1.upgrd.4 = bitcast i8* %tmp1 to i16*
   %tmp = load i16, i16* %tmp1.upgrd.4
   %tmp6 = call i16 @llvm.bswap.i16( i16 %tmp )
   ret i16 %tmp6
 }
 
+; TODO: combine the bswap feeding a store on subtargets
+; that do not have an STDBRX.
 define void @STDBRX(i64 %i, i8* %ptr, i64 %off) {
 ; PWR7_32-LABEL: STDBRX:
 ; PWR7_32:       # %bb.0:
 ; PWR7_32-NEXT:    li r6, 4
 ; PWR7_32-NEXT:    add r7, r5, r8
 ; PWR7_32-NEXT:    stwbrx r4, r5, r8
 ; PWR7_32-NEXT:    stwbrx r3, r7, r6
 ; PWR7_32-NEXT:    blr

 ; PWR7_32-NEXT:    li r5, 4
 ; PWR7_32-NEXT:    add r7, r3, r6
 ; PWR7_32-NEXT:    lwbrx r4, r3, r6
 ; PWR7_32-NEXT:    lwbrx r3, r7, r5
 ; PWR7_32-NEXT:    blr
 ;
 ; X64-LABEL: LDBRX:
 ; X64:       # %bb.0:
-; X64-NEXT:    ldx r4, r3, r4
-; X64-NEXT:    rotldi r5, r4, 16
-; X64-NEXT:    rotldi r3, r4, 8
-; X64-NEXT:    rldimi r3, r5, 8, 48
-; X64-NEXT:    rotldi r5, r4, 24
-; X64-NEXT:    rldimi r3, r5, 16, 40
-; X64-NEXT:    rotldi r5, r4, 32
-; X64-NEXT:    rldimi r3, r5, 24, 32
-; X64-NEXT:    rotldi r5, r4, 48
-; X64-NEXT:    rldimi r3, r5, 40, 16
-; X64-NEXT:    rotldi r5, r4, 56
-; X64-NEXT:    rldimi r3, r5, 48, 8
-; X64-NEXT:    rldimi r3, r4, 56, 0
+; X64-NEXT:    li r5, 4
+; X64-NEXT:    lwbrx r6, r3, r4
+; X64-NEXT:    add r3, r3, r4
+; X64-NEXT:    lwbrx r3, r3, r5
+; X64-NEXT:    rldimi r3, r6, 32, 0
 ; X64-NEXT:    blr
 ;
 ; PWR7_64-LABEL: LDBRX:
 ; PWR7_64:       # %bb.0:
 ; PWR7_64-NEXT:    ldbrx r3, r3, r4
 ; PWR7_64-NEXT:    blr
   %tmp1 = getelementptr i8, i8* %ptr, i64 %off
   %tmp1.upgrd.2 = bitcast i8* %tmp1 to i64*
   %tmp = load i64, i64* %tmp1.upgrd.2
   %tmp14 = tail call i64 @llvm.bswap.i64( i64 %tmp )
   ret i64 %tmp14
 }
 
 declare i16 @llvm.bswap.i16(i16)
 declare i32 @llvm.bswap.i32(i32)
 declare i64 @llvm.bswap.i64(i64)

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 < %s | \
+; RUN:   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
+}
+
+define i64 @misaligned_volatile(i64* %p) {
+; CHECK-LABEL: misaligned_volatile:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    ld 4, 0(3)
+; CHECK-NEXT:    rotldi 5, 4, 16
+; CHECK-NEXT:    rotldi 3, 4, 8
+; CHECK-NEXT:    rldimi 3, 5, 8, 48
+; CHECK-NEXT:    rotldi 5, 4, 24
+; CHECK-NEXT:    rldimi 3, 5, 16, 40
+; CHECK-NEXT:    rotldi 5, 4, 32
+; CHECK-NEXT:    rldimi 3, 5, 24, 32
+; CHECK-NEXT:    rotldi 5, 4, 48
+; CHECK-NEXT:    rldimi 3, 5, 40, 16
+; CHECK-NEXT:    rotldi 5, 4, 56
+; CHECK-NEXT:    rldimi 3, 5, 48, 8
+; CHECK-NEXT:    rldimi 3, 4, 56, 0
+; CHECK-NEXT:    blr
+  %x = load volatile i64, i64* %p, align 1
+  %b = call i64 @llvm.bswap.i64(i64 %x)
+  ret i64 %b
+}
+declare i64 @llvm.bswap.i64(i64) #2