This is an archive of the discontinued LLVM Phabricator instance.

[LVI] Use Optional instead of out parameter (NFC)
ClosedPublic

Authored by nikic on Apr 17 2020, 9:56 AM.

Details

Summary

As suggested on D76788, this switches the LVI implementation to return Optional<ValueLatticeElement> from various methods, instead of passing in a ValueLatticeElement reference and returning a boolean.

Diff Detail

Event Timeline

nikic created this revision.Apr 17 2020, 9:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2020, 9:56 AM
nikic marked an inline comment as done.Apr 17 2020, 10:04 AM
nikic added inline comments.
lib/Analysis/LazyValueInfo.cpp
839

My C++ foo is weak... would the proper way to write this without copying the lattice value be TrueValue = *std::move(OptTrueVal)?

nikic marked an inline comment as done.Apr 17 2020, 10:12 AM
nikic added inline comments.
lib/Analysis/LazyValueInfo.cpp
839

Hm, it looks like moving an Optional moves the value (rather than making the Optional empty). As ValueLatticeElement does not specify a move constructor, I guess that means it would end up being copied, so that probably doesn't make sense.

fhahn added inline comments.Apr 17 2020, 12:15 PM
lib/Analysis/LazyValueInfo.cpp
839

Can this just be ValueLatticeElement &TrueVal?

nikic updated this revision to Diff 258515.Apr 18 2020, 6:45 AM
nikic marked an inline comment as done.

Use reference to ValueLatticeElement where possible.

nikic added inline comments.Apr 18 2020, 6:50 AM
lib/Analysis/LazyValueInfo.cpp
839

Right, I went with that variant now. The use of Optional will still make for a small but measurable regression (~0.1%), but that seems acceptable to me.

I'll also submit a followup to add a move constructor to ValueLatticeElement. It makes no difference for performance in practice, because we usually deal with APInts <= 64 bits, so move and copy constructor do about the same work. It would make a difference for large APInts.

fhahn accepted this revision.Apr 19 2020, 10:09 AM

LGTM, thanks for the cleanup!

This revision is now accepted and ready to land.Apr 19 2020, 10:09 AM
This revision was automatically updated to reflect the committed changes.