[PATCH] D70043: [ConstantRange] Add `mulWithNoWrap()` method

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

> But in the end, any suggestions on how to get *these* patches moving?

My opinion here hasn't really changed ... I still think that if we want to handle wrapping ranges more accurately, we need to do so in a more principled fashion that does not special case empty sets only. Specifically I would expect this to entail:

- Changing the `getClosestToZero()` helper to instead return two ranges, the positive and the negative one.
- Changing the signed part of the `multiply()` code to use those split ranges, instead of simple smin/smax.
- Possibly better, make the implementation of `mulWithNoWrap()` proper perform operations in the double bitwidth and then intersect with the legal part of the range. That way the empty set result should come about naturally as well.

It may well make sense to do this, but if we want to land something here more quickly, I would recommend:

- Remove the `EXPECT_EQ(CR.isEmptySet(), AllOverflow)` assertions and the code necessary to satisfy them. We already check that empty sets are correctly computed for non-wrapping sets in a separate assertion.
- Add some hand-written test coverage for `mulWithNoWrap()`. As the exhaustive tests are correctness-only, we should also check that we actually compute some reasonable ranges for a couple of inputs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70043