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

Repository
rL LLVM

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
35 ↗(On Diff #96466)

Missing assertion message

lib/Analysis/DemandedBits.cpp
89 ↗(On Diff #96466)

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

95 ↗(On Diff #96466)

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

lib/Analysis/InstructionSimplify.cpp
3356 ↗(On Diff #96466)

APInt::intersects?

4722 ↗(On Diff #96466)

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

lib/Analysis/ValueTracking.cpp
817 ↗(On Diff #96466)

APInt::intersects

2015 ↗(On Diff #96466)

APInt?

4264 ↗(On Diff #96466)

APInt::isSubsetOf ?

lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
115 ↗(On Diff #96466)

Update message - still refers to KnownZero and KnownOne

141 ↗(On Diff #96466)

braces

199 ↗(On Diff #96466)

APInt::intersects

231 ↗(On Diff #96466)

APInt::intersects

320 ↗(On Diff #96466)

APInt::intersects

403 ↗(On Diff #96466)

APInt::intersects

547 ↗(On Diff #96466)

APInt::intersects

lib/Transforms/InstCombine/InstructionCombining.cpp
2884 ↗(On Diff #96466)

KnownBits helper

lib/Transforms/Utils/SimplifyCFG.cpp
4384 ↗(On Diff #96466)

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 ↗(On Diff #96466)

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 ↗(On Diff #96610)

representing

lib/Analysis/ValueTracking.cpp
1399 ↗(On Diff #96610)

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

Known.Zero = Known2.Zero.reverseBits();
Known.One = Known2.One.reverseBits();
1404 ↗(On Diff #96610)

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 ↗(On Diff #96610)

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 ↗(On Diff #96610)

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 ↗(On Diff #96610)

I'll add the TODO

This revision was automatically updated to reflect the committed changes.