ValueTracking: Fix crash on weird size bswap

[arsenm]

Summary

APInt::byteSwap doesn't handle strange cases, so just give up on them.

[nikic]

I think it would make more sense to add support for this to APInt. These operations are well-defined and supported in IR, so APInt should support them as well.

[RKSimon]

In D74621#1876548, @nikic wrote:

I think it would make more sense to add support for this to APInt. These operations are well-defined and supported in IR, so APInt should support them as well.

+1

[arsenm]

In D74621#1876548, @nikic wrote:

I think it would make more sense to add support for this to APInt. These operations are well-defined and supported in IR, so APInt should support them as well.

I don't think this is really well defined for arbitrary odd cases. What does a bswap mean for non-byte multiples?

[nikic]

Sure, non-byte multiplies doesn't make sense, but byte multiples generally do. However, after checking LangRef we apparently do require the bitwidth to be a multiple of 16 for this intrinsic: https://llvm.org/docs/LangRef.html#llvm-bswap-intrinsics

[RKSimon]

In D74621#1876606, @nikic wrote:

Sure, non-byte multiplies doesn't make sense, but byte multiples generally do. However, after checking LangRef we apparently do require the bitwidth to be a multiple of 16 for this intrinsic: https://llvm.org/docs/LangRef.html#llvm-bswap-intrinsics

Whether the intrinsic does or not, adding support to APInt is pretty trivial. Scarily we don't appear to have any unit tests at all for APInt:byteSwap

[nikic]

So the APInt support is there now, but we still have to do something here. I would suggest to enforce this requirement in the IR verifier (it already has a bunch of similar checks in visitIntrinsicCall), so that individual code handling bswaps doesn't have to deal with it.

[arsenm]

In D74621#1878391, @nikic wrote:

So the APInt support is there now, but we still have to do something here. I would suggest to enforce this requirement in the IR verifier (it already has a bunch of similar checks in visitIntrinsicCall), so that individual code handling bswaps doesn't have to deal with it.

I would suggest to eliminate the restriction from the langref. The GlobalISel legalizer at least handles the case just fine

In D74621#1878391, @nikic wrote:

So the APInt support is there now, but we still have to do something here. I would suggest to enforce this requirement in the IR verifier (it already has a bunch of similar checks in visitIntrinsicCall), so that individual code handling bswaps doesn't have to deal with it.

Verifier check added in b76bbcc60db6cd7d42bf46f2fcdc6784250554d7

llvm/lib/Analysis/ValueTracking.cpp

if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
       switch (II->getIntrinsicID()) {
       default: break;
       case Intrinsic::bitreverse:
         computeKnownBits(I->getOperand(0), Known2, Depth + 1, Q);
         Known.Zero |= Known2.Zero.reverseBits();
         Known.One |= Known2.One.reverseBits();
         break;
       case Intrinsic::bswap:
+        // APInt::byteSwap doesn't handle these cases
+        if (BitWidth < 16 || BitWidth % 16 != 0)
+          break;
+
         computeKnownBits(I->getOperand(0), Known2, Depth + 1, Q);
         Known.Zero |= Known2.Zero.byteSwap();
         Known.One |= Known2.One.byteSwap();
         break;
       case Intrinsic::ctlz: {
         computeKnownBits(I->getOperand(0), Known2, Depth + 1, Q);
         // If we have a known 1, its position is our upper bound.
         unsigned PossibleLZ = Known2.One.countLeadingZeros();

llvm/test/Analysis/ValueTracking/bswap.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -instsimplify -S < %s | FileCheck %s
+
+; Make sure there's no assert on weird sized bswaps
+
+declare i8 @llvm.bswap.i8(i8) #1
+declare i24 @llvm.bswap.i24(i24) #1
+
+define i8 @bswap_i8(i8 %src) {
+; CHECK-LABEL: @bswap_i8(
+; CHECK-NEXT:    [[BSWAP:%.*]] = call i8 @llvm.bswap.i8(i8 [[SRC:%.*]])
+; CHECK-NEXT:    ret i8 [[BSWAP]]
+;
+  %bswap = call i8 @llvm.bswap.i8(i8 %src)
+  ret i8 %bswap
+}
+
+define i24 @bswap_i24(i24 %src) {
+; CHECK-LABEL: @bswap_i24(
+; CHECK-NEXT:    [[BSWAP:%.*]] = call i24 @llvm.bswap.i24(i24 [[SRC:%.*]])
+; CHECK-NEXT:    ret i24 [[BSWAP]]
+;
+  %bswap = call i24 @llvm.bswap.i24(i24 %src)
+  ret i24 %bswap
+}
+
+attributes #1 = { nounwind readnone speculatable willreturn }