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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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));
----------------
Is this intentionally different than the usual "N0.hasOneUse()"?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9599
+        TLI.isTruncateFree(VT, HalfVT) &&
+        (!LegalOperations || hasOperation(ISD::BSWAP, HalfVT))) {
+      SDValue Res = N0.getOperand(0);
----------------
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

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120192