[PATCH] D103788: [InstCombine] Eliminate casts to optimize ctlz operation

[Datta Nagraj via Phabricator via llvm-commits]

datta.nagraj marked 2 inline comments as done.
datta.nagraj added a comment.

@spatel Addressed review comments.



================
Comment at: llvm/test/Transforms/InstCombine/zext-ctlz-trunc-to-ctlz-add.ll:53
+
+define i32 @trunc_ctlz_zext_i32_i64(i32 %x) {
+; CHECK-LABEL: @trunc_ctlz_zext_i32_i64(
----------------
spatel wrote:
> There are a lot of tests here that don't provide much extra coverage. These are only changing the types to confirm that we get the add constant correct? I think we can verify that with 2 tests of varying types (including a vector type), but we can do better by using a weird type (for example 3 x i17). We can also get more value by varying the 2nd parameter (make it `true` half of the time?).
Removed redundant tests. (Yes, they were checking for the add constant)
Added weird type tests.
Varied the 2nd parameter as true for half of them.


================
Comment at: llvm/test/Transforms/InstCombine/zext-ctlz-trunc-to-ctlz-add.ll:215
+  %x1 = trunc i32 %p to i16
+  %y = trunc i32 %p to i16
+  %zz = add nsw i16 %x1, %y
----------------
spatel wrote:
> Typically, we make the extra use minimal and more plausible by adding a `call void @use(i32 %p)` instruction.
> Also, what happens if the zext has an extra use?
Made use of the function call. That's a nice idea, didn't think of that.
Added condition check for extra use of zext and added a test case for that as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103788