This is an archive of the discontinued LLVM Phabricator instance.

Add support for computing "zext of value" in KnownBits. NFCI
ClosedPublic

Authored by bjope on Feb 25 2019, 3:29 PM.

Details

Summary

The description of KnownBits::zext() and
KnownBits::zextOrTrunc() has confusingly been telling
that the operation is equivalent to zero extending the
value we're tracking. That has not been true, instead
the user has been forced to explicitly set the extended
bits as known zero afterwards.

This patch adds a second argument to KnownBits::zext()
and KnownBits::zextOrTrunc() to control if the extended
bits should be considered as known zero or as unknown.

Event Timeline

bjope created this revision.Feb 25 2019, 3:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2019, 3:29 PM
bjope updated this revision to Diff 188269.Feb 25 2019, 4:02 PM

Minor cleanup.

RKSimon added inline comments.Feb 27 2019, 5:42 AM
llvm/include/llvm/Support/KnownBits.h
123

if (BitWidth > OldBitWidth && ExtendedBitsAreKnownZero)

bjope marked an inline comment as done.Feb 27 2019, 6:00 AM
bjope added inline comments.
llvm/include/llvm/Support/KnownBits.h
123

The calls to Zero.zext(...) and One.zext(...) will assert if not increasing the width. You need to use zextOrTrunc (maybe there is a zextOrSelf as well) if it should be allowed to have the same size.

I guess that is why these methods hasn't been asserting themselves earlier. Should I add an explicit assert here instead of a condition that always should be fulfilled? Or is it OK as is, given this answer?

RKSimon accepted this revision.Feb 28 2019, 5:15 AM

Yup, its OK as given. LGTM cheers!

This revision is now accepted and ready to land.Feb 28 2019, 5:15 AM
This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Feb 28 2019, 8:44 AM
nikic added inline comments.
llvm/trunk/include/llvm/Support/KnownBits.h
120 ↗(On Diff #188741)

Might be clearer to separate this into a zext and an anyext method?

bjope added inline comments.Feb 28 2019, 10:49 AM
llvm/trunk/include/llvm/Support/KnownBits.h
120 ↗(On Diff #188741)

Simply changing the semantics of zext in a single step seemed a little bit risky (at least considering OOT targets).
We might wanna do that split in the future, assuming that OOT targets have had time to adapt to this two-argument version first. That is also why I decided not to use a default value for ExtendedBitsAreKnownZero.

One thing that I'm not sure of is if these methods that map directly to the APInt names are supposed to indicate that we apply the APInt function on One and Zero (i.e. the name should map to the operation applied on the underlying bitmaps)? We already have computeForAddSub that is named based on the operations applied to the tracked value. Maybe we should add computeForZext and computeForSext instead, to make the naming more consistent for methods that refer to the operation on the tracked value.

RKSimon added inline comments.Feb 28 2019, 1:55 PM
llvm/trunk/include/llvm/Support/KnownBits.h
120 ↗(On Diff #188741)

I agree with @bjope - zext is pretty ambiguous, so including the explicit bool flag makes sense to me. 'zextKnownZeroBits()' and 'zextUnknownBits()' style methods would be alternatives but aren't any better afaict.