Please merge fd773e8a51b8 into 10.0.1

[llvmbot]

Bugzilla Link	45664
Resolution	FIXED
Resolved on	May 18, 2020 18:38
Version	trunk
OS	Windows NT
Blocks	#44654
Reporter	LLVM Bugzilla Contributor
CC	@zmodem,@MaskRay,@tstellar
Fixed by commit(s)	fd773e8 aa97472

Extended Description

https://reviews.llvm.org/rGe98f73a629075ae3b9c4d5317bead5a122d69865

[llvmbot]

Correction, it should be https://reviews.llvm.org/rGfd773e8a51b82775f411061117173a21b500642a

[MaskRay]

Reasonable. Cherry picked to release/10.x

aa97472

[llvmbot]

Thank you!

[tstellar]

Reasonable. Cherry picked to release/10.x

https://github.com/llvm/llvm-project/commit/
aa97472

Fangrui, this patch introduces an ABI break[1] because of the change in size of the PendingLabelSections member. Are you able to modify the patch to avoid changing the ABI.

[1] https://github.com/llvm/llvm-project/runs/624609989

[llvmbot]

Ah my bad, I didn't think about the ABI.

on a 64-bit build,
sizeof(SmallVector<MCSection*, 2>) = 32 bytes
sizeof(SmallSetVector<MCSection *, 4>) = 88 bytes

Even if we reduce the number of inline items, it is still too big:
sizeof(SmallSetVector<MCSection *, 1>) = 48 bytes

The only option then is to revert and leave this for 11.0 ?

[tstellar]

Ah my bad, I didn't think about the ABI.

on a 64-bit build,
sizeof(SmallVector<MCSection*, 2>) = 32 bytes
sizeof(SmallSetVector<MCSection *, 4>) = 88 bytes

Even if we reduce the number of inline items, it is still too big:
sizeof(SmallSetVector<MCSection *, 1>) = 48 bytes

The only option then is to revert and leave this for 11.0 ?

Yes, I think so if it can't be re-written. I will revert it for now to get the tests back to green.

[MaskRay]

For release/10.x, we can keep the type of PendingLabelSections, but uniquify PendingLabelSections in lib/MC/MCObjectStreamer.cpp:flushPendingLabels()

---

This also raises a question whether 4 is the best constant.

SmallSetVector<MCSection *, 4> PendingLabelSections;

SmallVector + SmallDenseSet.

The 4 elements are duplicated in two containers, resulting in sizeof(MCSection*) * 4 * 2 = 64 bytes.

[llvmbot]

SmallSetVector layout
SmallSetVector<MCSection *, 4> is actually 88 bytes on a Release x64 build (see attached image). SmallVector is not as optimally implemented as the DenseMapBase, which has a 'small' bitflag. SmallVector doesn't, it always stores the pointer to the first element. I suppose this could be optimized at some point.

"4" is arbitrary. It could be anything else representative, but I don't think it really matters, unless it's a really high value, the SmallSetVector will probably resize itself in most cases.

[tstellar]

Merged: aa97472