This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] computeOverflowForSignedAdd and isKnownNonNegative
ClosedPublic

Authored by jingyue on Jul 17 2015, 2:26 PM.

Details

Summary

Refactor, NFC

Extracts computeOverflowForSignedAdd and isKnownNonNegative from NaryReassociate to ValueTracking in case
others need it.

Diff Detail

Event Timeline

jingyue updated this revision to Diff 30038.Jul 17 2015, 2:26 PM
jingyue retitled this revision from to [ValueTracking] computeOverflowForSignedAdd.
jingyue updated this object.
jingyue added a reviewer: reames.
jingyue updated this revision to Diff 30043.Jul 17 2015, 3:01 PM

extract isKnownNonNegative

jingyue retitled this revision from [ValueTracking] computeOverflowForSignedAdd to [ValueTracking] computeOverflowForSignedAdd and isKnownNonNegative.Jul 17 2015, 3:03 PM
jingyue updated this object.
reames added inline comments.Jul 17 2015, 4:04 PM
include/llvm/Analysis/ValueTracking.h
297–302

I'm not thrilled with the inconsistent interface with the ones above, but given the extra power, it's worth it.

You might separate this into two versions:

  1. which takes only LHS, RHS
  2. which takes only the add and extracts LHS, RHS

1 is useful for analysing llvm.sadd.with.overflow in instcombine.
2 is useful in your original case

Having them share an implementation is reasonable.

lib/Analysis/ValueTracking.cpp
3349

The comment here is stale. What you appear to be actually checking is that the sign of the Add is the same as at least one of the operands. That appears sufficient, but the comment is misleading.

jingyue updated this revision to Diff 30070.Jul 17 2015, 10:44 PM

two versions of computeOverflowForSignedAdd

reames accepted this revision.Aug 19 2015, 9:57 AM
reames edited edge metadata.

LGTM w/minor comments addressed. No further review needed.

include/llvm/Analysis/ValueTracking.h
302

/// doxygen comment please.

305

AC, CtxI, and DT need to be optional and defaulted to nullptr.

lib/Analysis/ValueTracking.cpp
3343

Use an early return to reduce nesting please.

3344

operands

lib/Transforms/Scalar/NaryReassociate.cpp
354

Doesn't need to be in this change, but this should be pushed inside the ValueTracking call and the wrapper function can probably completely disappear.

This revision is now accepted and ready to land.Aug 19 2015, 9:57 AM
majnemer added inline comments.
lib/Analysis/ValueTracking.cpp
3327

This seems to overlap with InstCombiner::WillNotOverflowSignedAdd, would it make sense to consolidate this with that?

reames added inline comments.Aug 19 2015, 11:19 AM
lib/Analysis/ValueTracking.cpp
3327

Didn't know we had that one. Yes, a follow up patch to consolidate would make great sense.

jingyue updated this revision to Diff 32714.Aug 20 2015, 11:08 AM
jingyue edited edge metadata.

rebase to master

jingyue marked 5 inline comments as done.Aug 20 2015, 11:12 AM

All comments addressed. Thanks Philip and David!

lib/Analysis/ValueTracking.cpp
3327

ACK. Will address this in a follow-up diff.

lib/Transforms/Scalar/NaryReassociate.cpp
354

Done in this patch.

jingyue closed this revision.Aug 20 2015, 11:27 AM
jingyue marked an inline comment as done.