[PATCH] D117680: [InstCombine] Simplify bswap -> shift

[Paweł Bylica via Phabricator via llvm-commits]

chfast added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1194
+
+    // bswap(x) -> shl(x) if x has at most 8 (byte) active bits
+    if (Known.countMaxActiveBits() <= 8)
----------------
craig.topper wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > Can this go further.
> > > 
> > > Something like
> > > 
> > > ```
> > > unsigned TZ = alignDown(Known.countMinTrailingZeros(), 8);
> > > unsigned LZ = alignDown(Known.countMinLeadingZeros(), 8);
> > > 
> > > if (BitWidth - TZ - LZ == 8) {
> > >   if ((BitWidth - LZ - 8) > TZ)
> > >     ShiftRight by ((BitWidth - LZ - 8)  - TZ)
> > >   else
> > >     ShiftLeft by (TZ - (BitWidth - LZ - 8))
> > > }
> > > ```
> > > 
> > > Adapted from the SimplifyDemandedBits for bswap like https://reviews.llvm.org/D117508
> > Oops that should be
> > 
> > ```
> > unsigned TZ = alignDown(Known.countMinTrailingZeros(), 8);
> > unsigned LZ = alignDown(Known.countMinLeadingZeros(), 8);
> > 
> > if (BitWidth - TZ - LZ == 8) {
> >   if ((BitWidth - TZ - 8) > TZ)
> >     ShiftRight by ((BitWidth - TZ - 8)  - TZ)
> >   else
> >     ShiftLeft by (TZ - (BitWidth - TZ - 8))
> > }
> > ```
> Or even simpler.
> 
> ```
> unsigned TZ = alignDown(Known.countMinTrailingZeros(), 8);
> unsigned LZ = alignDown(Known.countMinLeadingZeros(), 8);
> 
> if (BitWidth - TZ - LZ == 8) {
>   if (LZ > TZ)
>     ShiftRight by (LZ - TZ)
>   else
>     ShiftLeft by (TZ - LZ)
> }
> ```
> 
> Wish I could amend or delete my previous comments.
Yes, I ended up with the same simplification on paper. Although I think you still have the shift kinds swapped.


================
Comment at: llvm/test/Transforms/InstCombine/bswap-fold.ll:442
+; CHECK-NEXT:    [[TMP2:%.*]] = shl i64 [[TMP0:%.*]], 57
+; CHECK-NEXT:    [[TMP3:%.*]] = shl i64 [[TMP0]], 1
+; CHECK-NEXT:    [[TMP4:%.*]] = and i64 [[TMP3]], 254
----------------
spatel wrote:
> chfast wrote:
> > Here we replace bswap with `lshr i64 %2, 56`, but it is further expanded to 
> > ```
> > shl i64 %0, 1
> > and i64 %3, 254
> > ```
> I added one-use checks with:
> 2d031ec5e53f
> ...so this shouldn't have an extra instruction now.
> 
> Please pre-commit the tests with the baseline CHECKs, so we just show diffs in this patch.
> I added one-use checks with:
> 2d031ec5e53f
> ...so this shouldn't have an extra instruction now.
Nice

> Please pre-commit the tests with the baseline CHECKs, so we just show diffs in this patch.
Will do.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117680