This is an archive of the discontinued LLVM Phabricator instance.

[ValueLattice] Use union to shave off ptr size bytes from elements.
ClosedPublic

Authored by fhahn on Jan 10 2018, 5:23 AM.

Details

Summary

By using a union for Constant* and ConstantRange we can shave off ptr
size bytes off lattice elements. On 64 bit systems, it brings down the
size to 40 bytes from 48 bytes.

Initialization of Range happens on-demand using placement new, if the
state changes to constantrange from non-constantrange. Similarly, the
Range object is destroyed if the state changes from constantrange to
non-constantrange.

Diff Detail

Event Timeline

fhahn created this revision.Jan 10 2018, 5:23 AM
reames requested changes to this revision.Jan 17 2018, 6:53 PM

Bunch of minor comments, but once those addressed, likely good to go.

include/llvm/Analysis/ValueLattice.h
58

Minor: Calling this const is confusing. I keep reading this as "Const... Range" which is misleading. Val or ConstVal might be more clear.

84

You could initialize to undefined, then just invoke the operator= method on this.

86

Might make sense to add a ConstantRange copy constructor or move constructor. You have this idiom in a couple of places.

105

For error detection, it would be good to null out the Const field when switching away from something containing a Const.

110

Another instance of the move constructor.

182

Same point about nulling Const here.

235

You can use a move here. Or a move constructor if there is one.

275

These uses of cover functions should be NFC on their own. Please separate and commit to reduce the diff in next round of review.

This revision now requires changes to proceed.Jan 17 2018, 6:53 PM
fhahn updated this revision to Diff 130512.Jan 18 2018, 3:58 PM
fhahn marked 7 inline comments as done.

Thanks for having a look!

include/llvm/Analysis/ValueLattice.h
86

ConstantRange already has a copy constructor and I updated the code to use ìt: new (&Range) ConstantRange(Other.Range);. I hope that I understood what you meant correctly :)

davide accepted this revision.Jan 18 2018, 5:58 PM

LGTM after your changes but please wait for Philip.

LGTM after your changes but please wait for Philip.

In a case like this where I only had minor comments, please don't wait for me. If davade things this is good to go, run with it. I tend to be very busy and blocking anyone's forward progress on my availability is a bad idea.

reames accepted this revision.Jan 20 2018, 10:58 AM

Also, LGTM since I did have time to look through this time. :)

This revision is now accepted and ready to land.Jan 20 2018, 10:58 AM

Great, thanks for having a look!

This revision was automatically updated to reflect the committed changes.