[PATCH] D125603: [ConstantRange] Improve the implementation of binaryAnd

Nikita Popov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 16 07:18:05 PDT 2022


nikic added inline comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:1392
+      && getMaxPowOf2Factor(*Other.getSingleElement()).ugt(getUnsignedMax()))
+    return { APInt::getZero(getBitWidth()) };
 
----------------
In terms of general code structure, I'd do something like this:

```
if (const APInt *C = getSingleElement()) {
  if (const APInt *OtherC = Other.getSingleElement())
    return *C & *OtherC;

  // Your code here.
} else if (Other.isSingleElement()) {
  return Other.binaryAnd(*this);
}
```

This is to avoid to implement the single element + range case for both orders.

In https://github.com/llvm/llvm-project/commit/8ab819ad90d647b96bb4b6842a742d2552ba9e9c I've added a `toKnownBits()` method, which you should be able to use to implement a more general variant of this. I.e. to a `toKnownBits()` on the non-single-element range, and them together and then use fromKnownBits(). That should cover your case, a few additional ones, and also be more "obviously correct".


================
Comment at: llvm/test/Transforms/SCCP/range-and.ll:160
   %p = phi i64 [ %r.1, %bb1 ], [ %r.2, %bb2 ]
-  %p.and = and i64 %p, 512
+  %p.and = and i64 %p, 255
   %c = icmp ult i64 %p.and, 256
----------------
Why is this change being made?


================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:2535
+  EXPECT_EQ(R16_32.binaryAnd(R0_99), R0_32);
+  EXPECT_EQ(R0_99.binaryAnd(R16_32), R0_32);
+}
----------------
Please also include a call to TestBinaryOpExhaustive, to verify correctness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125603



More information about the llvm-commits mailing list