[PATCH] D142234: [LVI] Handle Intrinsic::ctlz

[Antonio Frighetto via Phabricator via llvm-commits]

antoniofrighetto added a comment.

@nikic, thank you, I'm having a look at these now. Just a few considerations.

1. I'm not entirely sure about the unit test failures: should the returned range simply not include zero, if the incoming interval may contain zero and `ZeroIsPoison` is true, as you noted? I reckoned that being more conservative (unknown/overdefined) could be better here. Same applies for `isEmptySet`, we may want to return overdefined, I guess (I'm looking at `Value of: CR.isEmptySet(), Actual: false, Expected: true`)?.
2. If we don't want to have `ctlz` API on APInt, could it be fine to overload `TestUnaryOpExhaustive` so that it accepts a callable reference to `std::optional<unsigned>(const APInt &)` too?
3. `range.ll` now fails as the metadata's range is no longer being used now. I'm gradually realizing that this needs to be handled properly in CVP as well. Could it be fine to revert the `ret i1 true` to this change <https://github.com/llvm/llvm-project/commit/4a3e006830aaaf094f3a8ea0a3780a5e1b6f3ecc>, considering that the range is now [0,16] (verification here <https://alive2.llvm.org/ce/z/pRmkWZ>) for this time, and postpone the handling in CVP for a separate dedicated patch?


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

https://reviews.llvm.org/D142234