This is an archive of the discontinued LLVM Phabricator instance.

[KnownBits] Move AND, OR and XOR logic into KnownBits
ClosedPublic

Authored by foad on Feb 5 2020, 8:25 AM.

Details

Summary

There are at least three clients for KnownBits calculations:
ValueTracking, SelectionDAG and GlobalISel. To reduce duplication the
common logic should be moved out of these clients and into KnownBits
itself.

This patch does this for AND, OR and XOR calculations by implementing
and using appropriate operator overloads KnownBits::operator& etc.

Diff Detail

Event Timeline

foad created this revision.Feb 5 2020, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2020, 8:25 AM
foad updated this revision to Diff 242638.Feb 5 2020, 8:35 AM

Updated a couple more clients.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

I'm not sure about this since the logic isn't completely obvious. What about KnowBits::computeForAND/computeForOR/computeForXOR helpers instead?

llvm/lib/Analysis/ValueTracking.cpp
1129

You have no equivalent to these comments which helps people understands whats going on.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

foad updated this revision to Diff 242646.Feb 5 2020, 8:57 AM

Add comments.
Update one more client.

foad marked an inline comment as done.Feb 5 2020, 9:04 AM

I'm not sure about this since the logic isn't completely obvious. What about KnowBits::computeForAND/computeForOR/computeForXOR helpers instead?

I think that's a matter of taste. Personally I find "computeForXXX" unnecessarily verbose in these simple cases. Also note that clients use both plain & and &= (and | and |= and ^ and ^=) and it's nice to have efficient move semantics in all cases. It seems to me that operator overloading provides the perfect framework for doing all of that.

On the other hand, thinking ahead a bit: add/subtract/multiply will still require a named method because they will take additional arguments like nsw and nuw. Shifts might require named methods so that we can reasonably distinguish signed from unsigned right shifts.

Unit tests: unknown.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

foad added a comment.Feb 17 2020, 5:28 AM

Ping!

Any other opinions on whether it's a good idea to use operator overloading here? I would like to point out that I'm trying to follow the example of APInt, which uses operator overloading where it makes sense. :-)

nikic added a comment.Feb 17 2020, 5:33 AM

Imho this use of operator-overloading is fine. I think it's understood that operations on KnownBits always mean "what are the known bits after performing this operation", not "apply this operation element-wise", so I think there's little potential for confusion here.

lebedev.ri accepted this revision.Apr 8 2020, 12:52 PM

Sorry for the delay, this looks like a reasonable cleanup to me.
Thanks.

llvm/lib/Support/KnownBits.cpp
86–104

s/Each result/Result/

This revision is now accepted and ready to land.Apr 8 2020, 12:52 PM
RKSimon added inline comments.Apr 8 2020, 2:02 PM
llvm/lib/Analysis/ValueTracking.cpp
1107

Please can you put these output description comments into the respective KnownBits implementation

foad updated this revision to Diff 256213.Apr 9 2020, 1:47 AM
foad marked an inline comment as done.

Rebase and address review comment.

foad updated this revision to Diff 256214.Apr 9 2020, 1:52 AM

Fix rebase damage.

This revision was automatically updated to reflect the committed changes.