[PATCH] D74482: [KnownBits] Introduce anyext instead of passing a flag into zext

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

If I remember correctly one reason behind adding a non-default argument (introducing the kind of weird API) was to make sure any out-of-tree/downstream targets would get compilation faults, and not just a new behavior using the same old API.
Now I guess long enough time has passed, to remove those bools. And once again people downstream will know that they have to adjust things due to getting compilation faults.

Sorry if the below is a little bit out-of-context from your patch (just wanted to share some thoughts now that someone is looking into this class, but probably out of scope for this patch):

Does anyone know why all those methods are returning a new KnownBits object, without modifying the KnownBits that the method is invoked for.

I guess this has been to follow the same pattern as the underlying APInt objects, and I guess one goals is to make it possible to do things like this

  Known = Known.zext(X).trunc(Y).zextOrTrunc(Z):

But aren't we getting a lot of copying of KnownBits objects?

If we had an interface returning a reference to the current object, and removing the constness, we could just let these operation operated on the Zero/One members. And the above could be done as

  Known.zext(X).trunc(Y).zextOrTrunc(Z):

Or, given the existing use cases where I find it very rare to use the output chained like that , we could just let the methods return void.
So one would have to do it step by step:

  Known.zext(X);
  Known.trunc(Y);
  Known.zextOrTrunc(Z):

(maybe naming the methods using some convention such as `applyZext` or `computeForZext` etc. to indicate that we emulate that some operation is applied to the tracked value)

Not sure if that would be a major compile time improvement, but wouldn't we get rid of some copying of KnownBits objects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74482