[PATCH] D90924: [ConstantRange] Sign-flipping of signedness-invariant comparisons

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:159
+
+bool ConstantRange::areInsensitiveToSignednessOfSwappedICmpPredicate(
+    const ConstantRange &CR1, const ConstantRange &CR2) {
----------------
nikic wrote:
> Personally I'd convert this to `areInsensitiveToSignednessOfInvertedICmpPredicate()`, in which case the implementation would be the direct conjugate of the above, i.e.
> 
> ```
>   if (CR1.isEmptySet() || CR2.isEmptySet())
>     return true;
>         
>   return (CR1.isAllNonNegative() && CR2.isAllNegative()) ||
>          (CR1.isAllNegative() && CR2.isAllNonNegative());
> ```
> 
> I believe the only difference would be in the treatment of ranges that have a single element intersection (in which case the right canonicalization is probably towards an equality comparison rather than a different sign).
Hmm, tests suggest that said implementation is (also) precise.
I'm not seeing any obvious bugs in exhaustive test coverage,
so let's do that instead. Again, i don't really recall why this was
the way it it, perhaps i just got discouraged in non-applicability
of this in SCEV, and didn't get it fully cleaned up.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:171
+
+  return false;
+}
----------------
nikic wrote:
> This implementation reads really confused to me. You should be able to reduce it to just:
> ```
>   if (CR1.isEmptySet() || CR2.isEmptySet())
>     return true;
>   
>   return (CR1.isAllNonNegative() && CR2.isAllNonNegative()) ||
>          (CR1.isAllNegative() && CR2.isAllNegative());
> ```
> I checked that this still passes your tests.
Hmm, indeed. I don't really recall why i ended up with that,
i was looking for a complex solution, so i didn't end up
converging on the simplest one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90924