[PATCH] D90924: [ConstantRange] Sign-flipping of signedness-invariant comparisons
Roman Lebedev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Oct 31 11:25:44 PDT 2021
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
More information about the llvm-commits
mailing list