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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai 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
----------------
spatel wrote:
> 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();
Oh, I probably should have noticed that the if clause has a return. I'll update it. Thanks!


================
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())
----------------
spatel wrote:
> Do we need to check for 'volatile' or other modifiers?
> Are we ok with misaligned accesses?
> Should add regression tests either way.
There are no alignment requirements for the byte-reversed loads/stores. I'll add a test.


================
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:
----------------
spatel wrote:
> 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
> }
> 
> ```
Yes, the store combine is definitely a TODO :)
I'll update the tests, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104836