This is an archive of the discontinued LLVM Phabricator instance.

WholeProgramDevirt: For VCP use a 32-bit ConstantInt for the byte offset.
ClosedPublic

Authored by pcc on Feb 15 2017, 5:53 PM.

Details

Summary

A future change will cause this byte offset to be inttoptr'd and then exported
via an absolute symbol. On the importing end we will expect the symbol to be
in range [0,2^32) so that it will fit into a 32-bit relocation. The problem
is that on 64-bit architectures if the offset is negative it will not be in
the correct range once we inttoptr it.

This change causes us to use a 32-bit integer so that it can be inttoptr'd
(which zero extends) into the correct range.

Event Timeline

tejohnson added inline comments.Feb 16 2017, 1:51 PM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
934

What happens if OffsetByte is bigger than 2^32? Is that possible?

pcc added inline comments.Feb 17 2017, 10:52 AM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
934

Not without exceeding limits that are already in place. You would either need a vtable larger than 2^31 bytes (i.e. a vtable containing more than 2^29 (32-bit) or 2^28 (64-bit) virtual functions, which not only far exceeds the recommended minimum limits of the C++ standard, but by itself exceeds the limits of the small memory model) or enough VCP eligible virtual calls to cause the size of the vtable's constant region to extend beyond 2^31 (that would require at least 2^28 such vcalls, and most likely the instructions making up those vcalls would by themselves push the size beyond the small memory model limit).

And of course, neither scenario is very realistic.

tejohnson accepted this revision.Feb 17 2017, 11:15 AM

LGTM

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
934

I have seen a couple of apps that exceeded the small memory model, although it seems unlikely that we would have something with a vtable or virtual calls by themselves to hit this limit. Unlikely, but might be nice to have a reasonable failure mode if we need to go to to a larger memory model...

This revision is now accepted and ready to land.Feb 17 2017, 11:15 AM
This revision was automatically updated to reflect the committed changes.