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

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 2 inline comments as done.
nikic added inline comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:1018
+  if (!NegL.isEmptySet() && !NegR.isEmptySet()) {
+    // neg / neg = neg.
+    //
----------------
jdoerfert wrote:
> I'm confused, isn't `-3 / -3 = 1` ?
Right, this should read `neg / neg = pos`.


================
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);
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61238