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

Thu Jan 26 07:06:54 PST 2023

nikic added a comment.

In D142234#4082670 <https://reviews.llvm.org/D142234#4082670>, @antoniofrighetto wrote:

> @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`)?.

It's not necessary to make any conservative choices here. If ZeroIsPoison is set, you can assume that the input range does not contain zero, e.g. by intersecting it out.

> 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?

I think we should just create the APInt in the two places where it's needed.

> 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?

This issue should already be fixed by https://github.com/llvm/llvm-project/commit/2e9bc1b8614c9422573cf2f4728525787b0cb0cb.

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

https://reviews.llvm.org/D142234