This is an archive of the discontinued LLVM Phabricator instance.

Fix assertion in SmallDenseMap constructor with reserve from non-power-of-2 buckets count
ClosedPublic

Authored by ivafanas on Jul 14 2022, 7:38 PM.

Details

Summary

SmallDenseMap constructor with reserve gets an arbitrary NumInitBuckets value and passes it below to init method.

If NumInitBuckets is greater then InlineBuckets, then SmallDenseMap initializes to large representation passing NumInitBuckets below to DenseMap initialization. DenseMap::initEmpty method asserts that initial buckets count must be a power of 2.

Proposed solution is to update NumInitBuckets value in SmallDenseMap constructor till the next power of 2. It should satisfy both DenseMap preconditions and required minimum buckets count for reservation.

Diff Detail

Event Timeline

ivafanas created this revision.Jul 14 2022, 7:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 7:38 PM
ivafanas requested review of this revision.Jul 14 2022, 7:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 7:38 PM
atrick accepted this revision.Jul 14 2022, 9:21 PM

LGTM. I don't understand how no one has hit the assert. Or maybe they have and just changed their code to workaround it.

It's a little strange that the power-of-2 fix isn't handled directly in SmallDenseMap::init.

Someone might think it's strange that the initial size heuristic does not match DenseMap--but I don't think that matters here.

This revision is now accepted and ready to land.Jul 14 2022, 9:21 PM

LGTM ...

Could you please help me to commit this?
I do not have commit access to the repo.
Sorry for that :)

LGTM ...

Could you please help me to commit this?
I do not have commit access to the repo.
Sorry for that :)

Yeah, but I don't work on LLVM main. It'll take me some time to set things up--not even sure of the current process. Can another reviewer do this quickly?

LGTM ...

Could you please help me to commit this?
I do not have commit access to the repo.
Sorry for that :)

Yeah, but I don't work on LLVM main. It'll take me some time to set things up--not even sure of the current process. Can another reviewer do this quickly?

Sure, I'll get it

This revision was landed with ongoing or failed builds.Jul 25 2022, 10:10 AM
This revision was automatically updated to reflect the committed changes.