This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Introduce a KnownBits struct to wrap the two APInts for computeKnownBits
ClosedPublic

Authored by craig.topper on Apr 21 2017, 2:38 PM.

Details

Summary

This patch introduces a new KnownBits struct that wraps the two APInt used by computeKnownBits. This allows us to treat them as more of a unit.

Initially I've just altered the signatures of computeKnownBits and InstCombine's simplifyDemandedBits to pass a KnownBits reference instead of two separate APInt references. I'll do similar to the SelectionDAG version of computeKnownBits/simplifyDemandedBits as a separate patch.

I've added a constructor that allows initializing both APInts to the same bit width with a starting value of 0. This reduces the repeated pattern of initializing both APInts. Once place default constructed the APInts so I added a default constructor for those cases.

Going forward I would like to add more methods that will work on the pairs. For example trunc, zext, and sext occur on both APInts together in several places. We should probably add a clear method that can be used to clear both pieces. Maybe a method to check for conflicting information. A method to return (Zero|One) so we don't write it out everywhere. Maybe a method for (Zero|One).isAllOnesValue() to determine if all bits are known. I'm sure there are many other methods we can come up with.

Diff Detail

Event Timeline

craig.topper created this revision.Apr 21 2017, 2:38 PM

Forgot to include the header file for the KnownBits struct.

RKSimon edited edge metadata.Apr 25 2017, 3:51 AM

I like this but there is a lot going on in the patch and I had to stop myself adding more comments....

Some of my comments aren't going to help reduce the patch as they're aren't directly related to the addition of the KnownBits, but you have quite a bit of refactoring (to APInt helper methods) in here already that should probably be done as NFC pre-commits.

I also don't think you need to include KnownBits.h in most places if you included it in ValueTracking.h ?

include/llvm/Support/KnownBits.h
36

Missing assertion message

lib/Analysis/DemandedBits.cpp
89

Known = KnownBits(BitWidth) ? (and in other cases below)

94

Known2 = KnownBits(BitWidth) ? (and in other cases)

lib/Analysis/InstructionSimplify.cpp
3369

APInt::intersects?

4736

This might be worth a convenience method as we often want to determine if KnownBits covers all bits.

lib/Analysis/ValueTracking.cpp
817

APInt::intersects

2015

APInt?

4264

APInt::isSubsetOf ?

lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
114–115

Update message - still refers to KnownZero and KnownOne

141

braces

197

APInt::intersects

229

APInt::intersects

318

APInt::intersects

401

APInt::intersects

545

APInt::intersects

lib/Transforms/InstCombine/InstructionCombining.cpp
2884

KnownBits helper

lib/Transforms/Utils/SimplifyCFG.cpp
4384

APInt::intersects and APInt::isSubsetOf

I considered adding KnownBits.h to ValueTracking.h, but I wasn't sure if we wanted to drag APInt.h into every file thjat included ValueTracking.h since there is other entry points in ValueTracking.h that do not use KnownBits.

lib/Transforms/InstCombine/InstructionCombining.cpp
2884

Prefer to leave to a future patch.

Update for all of the isSubsetOf and intersects being commited earlier.

Added message to assert in KnownBits.

Fix references to KnownZero and KnownOne in comments and assert messages.

craig.topper marked 9 inline comments as done.Apr 25 2017, 11:21 AM
RKSimon accepted this revision.Apr 26 2017, 3:54 AM

LGTM, with a few minor comments/queries.

include/llvm/Support/KnownBits.h
10

representing

lib/Analysis/ValueTracking.cpp
1399

Not due to this patch but shouldn't this be:

Known.Zero = Known2.Zero.reverseBits();
Known.One = Known2.One.reverseBits();
1404

Not due to this patch but shouldn't this be:

Known.Zero = Known2.Zero.byteSwap();
Known.One = Known2.One.byteSwap();
lib/Transforms/InstCombine/InstCombineCompares.cpp
179

computeSignedMinMaxValuesFromKnownBits and computeUnsignedMinMaxValuesFromKnownBits might be useful inside KnowBits someday if we need them in SelectionDAG as well? Worth a TODO?

This revision is now accepted and ready to land.Apr 26 2017, 3:54 AM
craig.topper added inline comments.Apr 26 2017, 9:24 AM
lib/Analysis/ValueTracking.cpp
1399

I believe I added the OR so that we would preserve bits computed from range metadata earlier.

But if you think that's incorrect we can certainly change it back and add a comment that it was intentionally different from the other intrinsics.

lib/Transforms/InstCombine/InstCombineCompares.cpp
179

I'll add the TODO

This revision was automatically updated to reflect the committed changes.