This is an archive of the discontinued LLVM Phabricator instance.

[KnownBits] Add implementation for `KnownBits::sdiv`
ClosedPublic

Authored by goldstein.w.n on May 7 2023, 11:57 PM.

Details

Summary

Can figure out some of the upper bits (similiar to udiv) if we know
the sign of the inputs.

As well, if we have the exact flag we can sometimes determine some
low-bits.

Diff Detail

Event Timeline

goldstein.w.n created this revision.May 7 2023, 11:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2023, 11:57 PM
goldstein.w.n requested review of this revision.May 7 2023, 11:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2023, 11:57 PM
RKSimon added inline comments.May 8 2023, 4:00 AM
llvm/lib/Support/KnownBits.cpp
538

assertion message

591

assert !Known.hasConflict()

nikic added a comment.May 8 2023, 4:02 AM

FWIW, if we know the sign bits of sdiv, we should really always be converting it to udiv. We actually already do this in CVP, and should also implement the same in InstCombine. It currently only handles the non-negative case, but the others can be implemented by adding the necessary negations.

FWIW, if we know the sign bits of sdiv, we should really always be converting it to udiv. We actually already do this in CVP, and should also implement the same in InstCombine. It currently only handles the non-negative case, but the others can be implemented by adding the necessary negations.

I saw that. It increases instruction count though. It's also hard to undo in the backend and results in worse codegen. I was planning to make a patch to actually stop doing it for non-constant operands because of that.

goldstein.w.n marked 2 inline comments as done.

Rebase

RKSimon accepted this revision.May 13 2023, 4:04 AM

LGTM - cheers

llvm/lib/Support/KnownBits.cpp
559

Xxact -> Exact

This revision is now accepted and ready to land.May 13 2023, 4:04 AM
goldstein.w.n marked an inline comment as done.May 13 2023, 10:53 AM
foad added a comment.May 18 2023, 2:47 AM

Sorry I'm late.

llvm/lib/Support/KnownBits.cpp
533

Typo "Equivalent"

541

Typo "Denom"

551

Why strictly positive here? I don't think you need to worry about divide-by-zero (poison) cases.

552

Typoe "Result is negative ..."

559

Typo "Result is negative ..."

569

At this point the sign of Res should match ResultSign, so you don't need a std::optional, you could just have a "bool ResultSignKnown".

572

Don't need this, or the makeNegative call below.

Sorry I'm late.

Shoot, this has already been pushed.

Would you like me to create a follow up patch? (likewise for the udiv patch).

foad added a comment.May 18 2023, 8:33 AM

Sorry I'm late.

Shoot, this has already been pushed.

Would you like me to create a follow up patch? (likewise for the udiv patch).

Sure, unless you think the changes are trivial enough not to need (pre-commit) review.

foad added a comment.May 18 2023, 8:34 AM

Sorry I'm late.

Shoot, this has already been pushed.

Would you like me to create a follow up patch? (likewise for the udiv patch).

Just FYI, on matters of process:
https://llvm.org/docs/CodeReview.html#must-code-be-reviewed-prior-to-being-committed
https://llvm.org/docs/CodeReview.html#can-code-be-reviewed-after-it-is-committed

goldstein.w.n marked 2 inline comments as done.May 18 2023, 5:42 PM

Sorry I'm late.

Shoot, this has already been pushed.

Would you like me to create a follow up patch? (likewise for the udiv patch).

Just FYI, on matters of process:
https://llvm.org/docs/CodeReview.html#must-code-be-reviewed-prior-to-being-committed
https://llvm.org/docs/CodeReview.html#can-code-be-reviewed-after-it-is-committed

Created D150921, D150922, and D150923 think addressed all your concerns except the ones explicitly commented there.