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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15245
       return SDValue(N, 0);
+    } else if (is64BitBswapOn64BitTgt && isSingleUseNonExtLd) {
+      // Convert this to two 32-bit bswap loads and a BUILD_PAIR. Do this only
----------------
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();


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15246-15247
+    } 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())
----------------
Do we need to check for 'volatile' or other modifiers?
Are we ok with misaligned accesses?
Should add regression tests either way.


================
Comment at: llvm/test/CodeGen/PowerPC/ld-bswap64-no-ldbrx.ll:3
+; RUN: llc -mtriple=powerpc64-- -mcpu=pwr5 -verify-machineinstrs | FileCheck %s
+define void @bs(i64* %p) {
+; CHECK-LABEL: bs:
----------------
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
}

```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104836/new/

https://reviews.llvm.org/D104836