[PATCH] D72423: [DemandedBits] Improve accuracy of Add propagator

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

This seems okay to me in general, assuming i'm not misreading the tests and they are actually exhaustive.

In D72423#1959341 <https://reviews.llvm.org/D72423#1959341>, @RKSimon wrote:

> Adding @regehr and @nikic as they might have some thoughts on this




> If possible I'd like to see some tests in InstCombine/InstSimplify that demonstrates the benefit of this.

I too would like to see some such test.

> Does anyone have comments about the exhaustive tests? 4 bits shouldn't take very long to complete but I'm not sure about general policy on this approach.

That is fine by me. This is what is being done for all the good ConstantRange tests already anyway.



================
Comment at: llvm/include/llvm/Analysis/DemandedBits.h:64-66
+  /// Compute alive bits of one addition operand from alive output and known
+  /// operand bits
+  static APInt determineLiveOperandBitsAdd(unsigned OperandNo,
----------------
I find the naming scheme confusing. Why [a]live?
What we are deducing here, is what bits of operand OperandNo
are actually *demanded* given the demanded bits of result of operation, no?


================
Comment at: llvm/lib/Analysis/DemandedBits.cpp:482-483
+                                              const KnownBits &RHS,
+                                              bool CarryZero, bool CarryOne) {
+  // Boundary bits' carry out is unaffected by their carry in.
+  APInt Bound = (LHS.Zero & RHS.Zero) | (LHS.One & RHS.One);
----------------
Assert only one carry is selected?


================
Comment at: llvm/unittests/IR/DemandedBitsTest.cpp:17-38
+template <typename Fn>
+static void EnumerateKnownBits(unsigned Bits, Fn TestFn) {
+  unsigned Max = 1 << Bits;
+  for (unsigned Zero = 0; Zero < Max; Zero++) {
+    for (unsigned One = 0; One < Max; One = ((One | Zero) + 1) & ~Zero) {
+      KnownBits Known;
+      Known.Zero = APInt(Bits, Zero);
----------------
I think there's some intersection with helpers defined in `KnownBitsTest.cpp`
It would be best to move them into one common header (`KnownBitsTest.h`?) and use them consistently.


================
Comment at: llvm/unittests/IR/DemandedBitsTest.cpp:18
+template <typename Fn>
+static void EnumerateKnownBits(unsigned Bits, Fn TestFn) {
+  unsigned Max = 1 << Bits;
----------------
I think this is just `ForeachKnownBits()` from `KnownBitsTest.cpp`?


================
Comment at: llvm/unittests/IR/DemandedBitsTest.cpp:31
+template <typename Fn>
+static void EnumerateRemainingBits(const KnownBits &Known, Fn TestFn) {
+  unsigned Max = 1 << Known.getBitWidth();
----------------
This looks like to be `ForeachNumInKnownBits()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72423