[PATCH] D112298: [InstCombine] Generalize sadd.sat combine to compute sign bits.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

LGTM, thank you.



================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:206-208
+  /// Get the minimum bit size for this Value \p Op as a signed integer.
+  /// i.e. The minimum bitwidth where the sign of the value is known to remain
+  /// the same.
----------------
Hm, i think this isn't strictly precise.
I.e. given `0b101` which is negative, we can't truncate it to `0b1`,
even thought that doesn't change the sign. So i think you should either
talk about the algebraic value, or take inspiration from how langref,
and say something like
```
/// Get the minimum bit size for this Value \p Op as a signed integer.
/// i.e.  x == sext(trunc(x to MinSignedBits) to bitwidth(x))
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1273-1274
   // the operands of the add are 64 bits wide, we need at least 33 sign bits.
-  unsigned NeededSignBits = CI1->getBitWidth() - NewWidth + 1;
-  if (IC.ComputeNumSignBits(A, 0, &I) < NeededSignBits ||
-      IC.ComputeNumSignBits(B, 0, &I) < NeededSignBits)
+  if (IC.ComputeMinSignedBits(A, 0, &I) > NewWidth ||
+      IC.ComputeMinSignedBits(B, 0, &I) > NewWidth)
     return nullptr;
----------------
I think this is a NFC change, in which case can you precommit it first?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2318-2319
+  // is usually achieved via a sext from a smaller type.
+  if (ComputeMinSignedBits(AddSub->getOperand(0), 0, AddSub) > NewBitWidth ||
+      ComputeMinSignedBits(AddSub->getOperand(1), 0, AddSub) > NewBitWidth)
+    return nullptr;
----------------
These aren't using InstCombine's wrapper?


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

https://reviews.llvm.org/D112298