This is an archive of the discontinued LLVM Phabricator instance.

[KnownBits] Introduce anyext instead of passing a flag into zext
ClosedPublic

Authored by foad on Feb 12 2020, 5:44 AM.

Details

Summary

This was a very odd API, where you had to pass a flag into a zext
function to say whether the extended bits really were zero or not. All
callers passed in a literal true or false.

I think it's much clearer to make the function name reflect the
operation being performed on the value we're tracking (rather than on
the KnownBits Zero and One fields), so zext means the value is being
zero extended and new function anyext means the value is being extended
with unknown bits.

NFC.

Diff Detail

Event Timeline

foad created this revision.Feb 12 2020, 5:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2020, 5:44 AM
bjope added a comment.Feb 12 2020, 7:37 AM

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.

foad added a comment.Feb 12 2020, 7:59 AM

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?

Yes, but I think the way to fix that is to add better support for move semantics. APInt already does quite a bit of this.

bjope added a comment.Feb 12 2020, 9:27 AM

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?

Yes, but I think the way to fix that is to add better support for move semantics. APInt already does quite a bit of this.

Sure. You are probably right. Move semantics could solve that problem in a better way.

bjope accepted this revision.Feb 12 2020, 9:38 AM

LGTM!

This revision is now accepted and ready to land.Feb 12 2020, 9:38 AM
This revision was automatically updated to reflect the committed changes.