[PATCH] D156029: [InstCombine] icmp udiv transform

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4325
+                           InstCombiner::BuilderTy &Builder) {
+  if (!Cmp.isUnsigned() && !Cmp.isEquality())
+    return nullptr;
----------------
!isSigned?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4339
+    IntegerBitWidth = ExtendedType->getIntegerBitWidth();
+  }
+
----------------
GetScalarSizeInBits?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4341
+
+  if (IntegerBitWidth > 128)
+    return nullptr;
----------------
Why the > 128 threshold?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4370
+  // result = is_less || is_greater
+  //
+
----------------
This really needs alive2 links. Can you please add to the summary.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4374
+  Value *Y = Builder.CreateZExt(UDiv->getOperand(1), ExtendedType);
+  Value *Z = Builder.CreateZExt(Cmp.getOperand(1), ExtendedType);
+
----------------
Why only support udiv as operand 0?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4376
+
+  Value *ConstantOne = ConstantVector::getIntegerValue(
+      ExtendedType, APInt::getOneBitSet(IntegerBitWidth, 0));
----------------
This isn't used by all predicates, can you only build this on the paths that req it?


================
Comment at: llvm/test/Transforms/InstCombine/icmp-udiv.ll:99
+; CHECK-NEXT:    [[TMP7:%.*]] = icmp ugt i128 [[TMP6]], [[TMP1]]
+; CHECK-NEXT:    [[TMP8:%.*]] = and i1 [[TMP5]], [[TMP7]]
+; CHECK-NEXT:    ret i1 [[TMP8]]
----------------
It's not clear this is more canonical or universally better. It's true we occasionally add instructions to avoid div, but this is a lot. Maybe this transform belongs in the backend?


================
Comment at: llvm/test/Transforms/InstCombine/unsigned-mul-overflow-check-via-udiv-of-allones.ll:111
   %t0 = udiv i8 -1, %x
   %r = icmp ugt i8 %t0, %y ; not ult
   ret i1 %r
----------------
Can you add some vector tests?


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

https://reviews.llvm.org/D156029