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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:1392
+      && getMaxPowOf2Factor(*Other.getSingleElement()).ugt(getUnsignedMax()))
+    return { APInt::getZero(getBitWidth()) };
 
----------------
nikic wrote:
> 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".
Hm, I thought that the known bits approach would be always more accurate than the below code if one operand is completely known, but that's not the case. E.g. `[0, 0b100] & -1` should be `[0, 0b100]` rather than `[0, 0b111]`.

I think we might be best off first computing a range using `fromKnownBits(toKnownBits() & toKnownBits())` and then applying the existing umin of max rule on top of that. That should be strictly better than the current implementation, and be reasonably accurate in practice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125603