This is an archive of the discontinued LLVM Phabricator instance.

[APInt] Give the value union a name so we can remove assumptions on VAL being the larger member
ClosedPublic

Authored by craig.topper on Mar 5 2017, 7:52 PM.

Details

Summary

Currently several places assume the VAL member is always at least the same size as pVal. In particular for a memcpy in the move assignment operator. While this is a true assumption, it isn't good practice to assume this.

This patch gives the union a name so we can write the memcpy in terms of the union itself. This also adds a similar memcpy to the move constructor where we previously just copied using VAL directly.

This patch is mostly just a mechanical addition of the U in front of VAL and pVAL everywhere. But several constructors had to be modified since we can't directly initializer a field of named union from the initializer list.

For the string constructor I pushed the VAL initialization down to where we know whether we need to use VAL or pVal. This leaves the upper bits of the union unitialized on a 32-bit target, but I'm not sure that's really a problem.

For the uint64_t constructor and the copy constructor I just removed the initialization of VAL to 0 altogether. pVal or VAL will always be assigned as needed. Again this makes the upper bits of the union garbage on 32-bit targets, but hopefully that's ok. This removal also makes the compiled binaries smaller because the compiler was unable to infer that the store is unnecessary on 64-bit target due to slow case init function being out of line.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Mar 5 2017, 7:52 PM
dblaikie accepted this revision.Mar 6 2017, 3:33 PM

Skimmed through most of it - looks good to me.

If you like, you could wait to see, or check with others, in case my idea that this is the Right Way to do this (having a named enum, etc) is correct/a shared opinion.

This revision is now accepted and ready to land.Mar 6 2017, 3:33 PM

Adding a few others who have been involved or reviewed APInt patch changes recently.

hans accepted this revision.Apr 17 2017, 11:56 AM

I don't know about The Right Way (tm), but this seems very reasonable to me. LGTM

RKSimon accepted this revision.May 3 2017, 2:31 AM

Any reason you haven't committed this yet? Makes sense to me too fwiw.

This revision was automatically updated to reflect the committed changes.