[PATCH] D29088: Do not create ctlz/cttz(X, false) when the target do not support zero defined ctlz/cttz.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In https://reviews.llvm.org/D29088#658651, @deadalnix wrote:

> In https://reviews.llvm.org/D29088#657697, @andreadb wrote:
>
> > I agree that we are currently missing an optimization.
> >  That said, (if I remember correctly) the only place where we form cttz/ctlz with `is_zero_undef=false` is in `foldSelectCttzCtlz()` and the only goal of that transform is to canonicalize cttz/ctlz in preparation for codegen. That's why I suggested considering the possibility of moving that transform into CGP. If we do this, then we no longer need to add extra optimization rules to "fix" the fact that we prematurely canonicalized.
>
>
> We also are doing it in InstCombine ( see foldCttzCtlz ) using isKnownNonZero, but that guy is unable to figure that one out. Looking at the implementation, it looks pretty ad hoc.


Just to clarify, `foldSelectCttzCtlz()` is the only place where we introduce cttz/ctlz with `is_zero_undef=false`.

We could potentially extend the logic in `isKnownNonZero` to add more cases. That said, I am not suggesting that we do that, since we can solve this entire issue by just moving the canonicalization rule from InstCombine to CGP.

Out of curiosity, I had a quick look at what extra rules would be needed to fix our reproducible. We would need to teach `isKnownNonZero` how to look through a xor (see below).

  ConstantInt *C
  if (match(V, m_Xor(m_Value(X), m_ConstantInt(C))))
    return isKnownNonEqual(X, C, Q);

However, function `isKnownNonEqual` would not know how to analyze users of X. So, we would need to add extra logic in ValueTracking.cpp to loop over the users of X in search of a dominating ICmpInst([ICMP_EQ|ICMP_NE], X, C)  (similarly to what function `isKnownNonNullFromDominatingCondition()` does).

So, although it is possible, it may not be the best way to fix this.


https://reviews.llvm.org/D29088