[PATCH] D74484: [AggressiveInstCombine] Add support for ICmp instr that feeds a select intsr's condition operand.

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

This is a lot more complicated than expected :(



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:41
+// This function assumes the constant is of a scalar type.
+static unsigned getConstMinBitWidthScl(bool IsSigned, Constant *C) {
+  assert(!C->getType()->isVectorTy() &&
----------------
I'd suggest Scl -> Scalar, we're not saving a lot of characters here :)


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:49
+  }
+  return false;
+}
----------------
`return false` doesn't make sense here. I believe that in the case where it is not analyzable, you need to return the bitwidth of the type. It should be possible to test this by making the constant a constant expression.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:81
+    if (Constant *C = dyn_cast<Constant>(V)) {
+      // If Op is a constant, make sure it fits it Ty is case it was provided.
+      if (Ty && Ty->getScalarSizeInBits() < getConstMinBitWidth(IsSigned, C))
----------------
nit: "fits in Ty in case"


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:109
+  // If one of them is a constant, return true.
+  if (HasConst)
+    return true;
----------------
Is it really sufficient that just one of them is a constant? Say we have `(zext i16 %x to i32) ult -1`, which is always true, converting it to `i16 %x ult -1` would no longer be always true.


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

https://reviews.llvm.org/D74484