Limit bswap and bitreverse matching to legal integer width

[vmustya]

Summary

The InstCombine pass matched a sequence of and-shl-add instructions into
bitreverse even when the demanded width is illegal for a target machine.
That produces the IR sequences like the following one:

%trunc = trunc i32 %x to i9
%rev = call i9 @llvm.bitreverse.i9(i9 %trunc)
%mask = and i9 %rev, -128
%reverse = zext i9 %mask to i32

The illegal bitreverse intrinsics are being emulated, so this combining
produces inefficient code.

[craig.topper]

llvm/test/Transforms/InstCombine/bitreverse.ll

265

The CHECK lines in this file are generated by a script, update_llc_test_checks.py. We shouldn't use CHECK-NOT in such tests as the script will just overwrite it the next time someone runs.

[vmustya]

I've updated LIT tests using the utils/update_test_checks.py tool.

[nikic]

Can the expansion produce better code than it does now, or does some important information get lost that prevents this?

Is the issue related to weird types like i9, or would this also be a problem for (say) i8?

[vmustya]

In D158548#4611202, @nikic wrote:

Can the expansion produce better code than it does now, or does some important information get lost that prevents this?

Is the issue related to weird types like i9, or would this also be a problem for (say) i8?

I've only seen the issue with the weird types like i9. I've added the minimal reproducer as a lit test case:

define i32 @illegal_width(i32 %x) {
  %b0 = and i32 %x, 1
  %b1 = and i32 %x, 2
  %shift1 = mul nuw nsw i32 %b1, 64
  %shift0 = shl nuw nsw i32 %b0, 8
  %reverse = add i32 %shift0, %shift1
  ret i32 %reverse
}

For x86_64 LLVM generates the following assembly:

	rol	di, 8
	mov	eax, edi
	shl	eax, 6
	and	eax, 16384
	shl	edi, 5
	and	edi, 16384
	lea	eax, [rdi + 2*rax]
	shr	eax, 7
	ret

With the patch applied, the assembly looks much better:

	mov	eax, edi
	and	eax, 2
	shl	eax, 6
	and	edi, 1
	shl	edi, 8
	or	eax, edi
	ret

[craig.topper]

In D158548#4611411, @vmustya wrote:

In D158548#4611202, @nikic wrote:

Can the expansion produce better code than it does now, or does some important information get lost that prevents this?

Is the issue related to weird types like i9, or would this also be a problem for (say) i8?

I've only seen the issue with the weird types like i9. I've added the minimal reproducer as a lit test case:

define i32 @illegal_width(i32 %x) {
  %b0 = and i32 %x, 1
  %b1 = and i32 %x, 2
  %shift1 = mul nuw nsw i32 %b1, 64
  %shift0 = shl nuw nsw i32 %b0, 8
  %reverse = add i32 %shift0, %shift1
  ret i32 %reverse
}

For x86_64 LLVM generates the following assembly:

	rol	di, 8
	mov	eax, edi
	shl	eax, 6
	and	eax, 16384
	shl	edi, 5
	and	edi, 16384
	lea	eax, [rdi + 2*rax]
	shr	eax, 7
	ret

With the patch applied, the assembly looks much better:

	mov	eax, edi
	and	eax, 2
	shl	eax, 6
	and	edi, 1
	shl	edi, 8
	or	eax, edi
	ret

The backend is expanding to i16 reverse sequence followed by a shift. Only 2 bits of that are demanded, but I suspect multiple uses prevents SimplifyDemandedBit from optimizing it.

There only 2 bits involved in the reverse here. Perhaps we shouldn't form bit reverse from something so sparse?

llvm/lib/Transforms/Utils/Local.cpp

if (auto *IVecTy = dyn_cast<VectorType>(ITy))
       DemandedTy = VectorType::get(DemandedTy, IVecTy);
   }
 
   // Check BitProvenance hasn't found a source larger than the result type.
   unsigned DemandedBW = DemandedTy->getScalarSizeInBits();
   if (DemandedBW > ITy->getScalarSizeInBits())
     return false;
 
+  // Check that the bswap/bitreverse will have legal bit width.
+  auto &DL = I->getModule()->getDataLayout();
+  if (DL.isIllegalInteger(DemandedBW))
+    return false;
+
   // Now, is the bit permutation correct for a bswap or a bitreverse? We can
   // only byteswap values with an even number of bytes.
   APInt DemandedMask = APInt::getAllOnes(DemandedBW);
   bool OKForBSwap = MatchBSwaps && (DemandedBW % 16) == 0;
   bool OKForBitReverse = MatchBitReversals;
   for (unsigned BitIdx = 0;
        (BitIdx < DemandedBW) && (OKForBSwap || OKForBitReverse); ++BitIdx) {
     if (BitProvenance[BitIdx] == BitPart::Unset) {

llvm/test/Transforms/InstCombine/bitreverse.ll

;
   %6 = and i64 %5, 558144
   %7 = or i64 %4, %6
   %8 = mul nuw nsw i64 %7, 65793
   %9 = lshr i64 %8, 16
   %10 = trunc i64 %9 to i8
   ret i8 %10
 }
 
+define i32 @illegal_width(i32 %x) {
+; CHECK-LABEL: @illegal_width
+; CHECK-NOT: call i9 @llvm.bitreverse.i9
+  %b0 = and i32 %x, 1
+  %b1 = and i32 %x, 2
+  %shift1 = mul nuw nsw i32 %b1, 64
+  %shift0 = shl nuw nsw i32 %b0, 8
+  %reverse = add i32 %shift0, %shift1
+  ret i32 %reverse
+}
+
 define i4 @shuf_4bits(<4 x i1> %x) {
 ; CHECK-LABEL: @shuf_4bits(
 ; CHECK-NEXT:    [[TMP1:%.*]] = bitcast <4 x i1> [[X:%.*]] to i4
 ; CHECK-NEXT:    [[CAST:%.*]] = call i4 @llvm.bitreverse.i4(i4 [[TMP1]])
 ; CHECK-NEXT:    ret i4 [[CAST]]
 ;
   %bitreverse = shufflevector <4 x i1> %x, <4 x i1> undef, <4 x i32> <i32 3, i32 2, i32 1, i32 0>
   %cast = bitcast <4 x i1> %bitreverse to i4