[PATCH] D61238: [ConstantRange] Add sdiv() support

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:1030
+    // form i4 [-3, -8] the next larger number exluding -8 would actually be -3
+    // rather than -7.
+    APInt Lo = (NegL.Upper - 1).sdiv(NegR.Lower);
----------------
nikic wrote:
> jdoerfert wrote:
> > Is it worth the hassle? Ignoring the special case and taking the APInt behavior seems like a much easier solution. Do we have evidence we don't want that? (I did not try to understand this special case yet!)
> Not handling this case would muddy the waters with regards to signs -- a negative number divided by a negative number should have a guaranteed positive result, and I would consider it important that we realize this.
> 
> The second motivation (and the reason why I'm going out of the way here to also treat wrapped ranges correctly), is that it allows us to exhaustively test the sdiv implementation for both conservative correctness //and// optimality, and thus give us full confidence in the implementation. Otherwise we would have to settle for conservative correctness only.
Agreed.

I think I understand the code below but it is hard to follow the logic and map it back to the comment above. Could you explain what is going on so it is easier to read, e.g.,

```
// Filter the singleton set [-1,0) as it does not make a defined contribution to the result.
if (!NegR.Lower.isAllOnesValue()) {
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61238