Page MenuHomePhabricator

[TableGen] Allow 2^63-1 and 2^63-2 as int literals.
ClosedPublic

Authored by simon_tatham on Mar 6 2019, 2:31 AM.

Details

Summary

These two values correspond to the 'Empty' and 'Tombstone' special
keys defined by DenseMapInfo<int64_t>, which means that neither one
can be used as a key in DenseMap<int64_t, anything>. Hence, if you try
to use either of those values as an int literal, IntInit::get() fails
an assertion when it tries to insert them into its static cache of
int-literal objects.

A simple workaround is to detect those two special values in advance
and cache them separately from the DenseMap.

Diff Detail

Repository
rL LLVM

Event Timeline

simon_tatham created this revision.Mar 6 2019, 2:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 2:31 AM

Instead of adding special cases for the tombstone, can we use some generic map that doesn't require user-defined tombstones, like std::map?

fhahn added a subscriber: fhahn.Mar 8 2019, 2:48 PM
fhahn removed a subscriber: fhahn.
fhahn added a subscriber: fhahn.

I'm happy to do that if you prefer – I left the DenseMap in place on the Chesterton's Fence principle. (I didn't know why it was originally chosen over std::map, so hesitated to unilaterally reverse the decision.)

Simpler version that just replaces DenseMap with std::map.

efriedma accepted this revision.Mar 11 2019, 11:05 AM

TableGen is not very performance sensitive; it's fine to just do the simple thing here.

DenseMap is generally faster than std::map both because it's a hashtable and because it allocates the keys and values inline... but clearly it's not a good idea to use it when it produces wrong results. Longer-term, we should probably come up with some alternative that has performance like a DenseMap, but doesn't misbehave for cases like this.

This revision is now accepted and ready to land.Mar 11 2019, 11:05 AM
This revision was automatically updated to reflect the committed changes.