[PATCH] D120192: [DAG] Attempt to fold bswap(shl(x,c)) -> zext(bswap(trunc(shl(x,c-bw/2))))

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9594
+  unsigned BW = VT.getScalarSizeInBits();
+  if (BW >= 32 && N0.getOpcode() == ISD::SHL && N->isOnlyUserOf(N0.getNode())) {
+    auto *ShAmt = dyn_cast<ConstantSDNode>(N0.getOperand(1));
----------------
spatel wrote:
> Is this intentionally different than the usual "N0.hasOneUse()"?
I'll change it - I'm just used to writing it that way to handle binops with duplicated ops.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9599
+        TLI.isTruncateFree(VT, HalfVT) &&
+        (!LegalOperations || hasOperation(ISD::BSWAP, HalfVT))) {
+      SDValue Res = N0.getOperand(0);
----------------
spatel wrote:
> If I'm seeing it correctly, we need to either tighten this clause by removing !LegalOperations or add the "BW % 16" requirement.
> 
> As-is (and we probably want a test either way), we could end up worse. For example on AArch64:
> 
> ```
> define i32 @test_bswap32_shift17(i32 %a0) {
>   %s = shl i32 %a0, 17
>   %b = call i32 @llvm.bswap.i32(i32 %s)
>   ret i32 %b
> }
> ```
> is:
> 
> ```
> 	lsl	w8, w0, #17
> 	rev	w0, w8
> 
> ```
> but this patch would make it:
> 
> ```
> 	lsl	w8, w0, #1
> 	rev	w8, w8
> 	lsr	w0, w8, #16
> 
> ```
Yeah, I figured allowing any shift amount was probably too far - I'll reduce it to (ShAmt %16) == 0 - its what I've been doing on the InstCombine variant that I'm still working on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120192