[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 inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:954
+  // trunc (ctlz_i32(zext(A), B) --> add(ctlz_i16(A, B), C)
+  if (match(Src, m_OneUse(m_Intrinsic<Intrinsic::ctlz>(
+                     m_OneUse(m_ZExt(m_Value(A))), m_Value(B))))) {
----------------
lebedev.ri wrote:
> Are you not missing a check that types of `A` and `Trunc` match?
Added the check and added a negative test for that as well.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:955
+  if (match(Src, m_OneUse(m_Intrinsic<Intrinsic::ctlz>(
+                     m_OneUse(m_ZExt(m_Value(A))), m_Value(B))))) {
+    Value *WidthDiff =
----------------
spatel wrote:
> I don't think we need this one-use check on the zext. Usually, we say that if we are not increasing the instruction count, but we are reducing the sequence of computation, it's ok to do the transform.
> 
> This will be easier to see if we commit the tests with the baseline CHECKs, so I pushed those to main:
> 9eef6e39816a
> 
> Please rebase and regenerate the CHECK lines (and see if it's ok to remove the 2nd one-use check).
Rebased on top of your change. Yes, seems that we don't need the second check. The test looks fine even without the second check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103788