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

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix added a comment.

Let's call it poor codegen if that is clearer. Indeed it isn't bad in the sense that it does something invalid, it is bad in the sense that it could be better. It doesn't seems to be a good idea to remove the branch and then reintroducing it later on in the case the target needs a branch anyway.

The reason is that the check for 0 is now implicit and, even if some optimization are done, it is very easy to defeat them. For instance, it is able to set the zero_undef flag when doing :

  if (n == 0) {
    // ...
  } else {
      ctlz(n);
  }

But not in trivial variations such as:

  if (n == 0xff) {
    // ...
  } else {
    ctlz(~n);
  }

This second case is a real one I faced when working on a serialization/deserialization library. While tweaking the code to get the optimization is possible, this would be brittle, and considering I faced variation of this problem several time, I though there is probably something to do here.

It doesn't seems that teaching all passes about this special case is going to fly. There are just too many cases and variations to match. All the code required to do these optimizations already exists, so I think it is smarter to leverage it.

Grepping quickly in there, it doesn't looks like SimplifyCFG create the select. I think we are good here. It may be profitable to explode the select into a branch/phi in that case, but that isn't suitable for InstCombine anyway as it preserve the control flow and it may not be necessary at all in the end, and improving the ability of the compiler to handle select seems like the right path anyway if that's the case at it'll be profitable to other structures as well.


https://reviews.llvm.org/D29088