[PATCH] D60264: [ConstantRange] Add isNegative() and isNonNegative() methods

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Looks good.

> The naming follows KnownBits, where these methods have the same meaning.
>  Could also use something like isAllNegative() and isAllNonNegative(), if that's more clear.

I think,
In `KnownBits` case, we either know signbit (and thus the sign), or we don't.
If we do know it, then we can answer if that `KnownBits` is negative / non-negative.

Here, in `ConstantRange`, we don't just know sign. We know *range*.
So negative/non-negative becomes ambigious, at least to me.
Does it mean that the *whole* range is like that (the intended meaning),
or only at least some part of the range?

So yes, i would personally prefer the variants with `All`.



================
Comment at: llvm/lib/IR/ConstantRange.cpp:393-395
+  // Empty set is all negative, full set is not.
+  if (Lower == Upper)
+    return Lower.isNullValue();
----------------
Hm, i would prefer to do
```
if(isEmptySet())
  return true; // Empty set is all-negative.
if(isFullSet())
  return false; // Full set is not all-negative.
```
since it is arguably more obvious.
Not sure it is better though.


================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:1648
+  EXPECT_TRUE(Empty.isNonNegative());
+
+  unsigned Bits = 4;
----------------
Can you also explicitly check full sets?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60264