-
Notifications
You must be signed in to change notification settings - Fork 11.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add known and demanded bits support for zext nneg #70858
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -1103,6 +1103,9 @@ static void computeKnownBitsFromOperator(const Operator *I, | |||
assert(SrcBitWidth && "SrcBitWidth can't be zero"); | ||||
Known = Known.anyextOrTrunc(SrcBitWidth); | ||||
computeKnownBits(I->getOperand(0), Known, Depth + 1, Q); | ||||
if (auto *Inst = dyn_cast<PossiblyNonNegInst>(I); | ||||
Inst && Inst->hasNonNeg() && !Known.isNegative()) | ||||
Known.makeNonNegative(); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is going to cause a conflict (and downstream assertion failures) if Known is known negative. You need to reset the known one bit -- or maybe adjust the makeNonNegative() API to do that itself, as this seems like an unnecessary footgun. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like doing it this way is missing an optimization. Be proving a conflict, haven't we proven the resulting value to be poison? If so, why are we not taking advantage of that to fold the value to poison? Glancing through the code for KnownBits and SimplifyDemanded, I don't see any clear evidence of a consistent scheme being followed. Do you know the big picture here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, a conflict implies the result is poison. I think the reason for the current approach is just "historical reasons" -- at some point somebody made the choice that we don't want to deal with conflicting KnownBits and we've stuck to that since. I'd be open to reevaluate that choice, but it's not something we can allow in just one place, it needs to be supported across all the KnownBits-based infrastructure-
Usually one of 1. locally resolve the conflict (e.g. here unset the negative bit or don't set non-negative), 2. return unknown bits or 3. return zero. See e.g.
!Known.isNegative() check first.
|
||||
Known = Known.zextOrTrunc(BitWidth); | ||||
break; | ||||
} | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit; imo should move this statement to before the if and use
&&
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you're suggesting? Why would I want to expand the scope of the Inst variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess the syntax
if (A = expr; other_stuff...)
is a pretty abnormal pattern and imo prone to be misread, but its a nit and nikics approved so no need to change if you feel otherwise.