Page MenuHomePhabricator

Extend function attributes bitset size from 64 to 96.
ClosedPublic

Authored by eugenis on Jul 12 2019, 1:03 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis created this revision.Jul 12 2019, 1:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 1:03 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

I've moved the bitset into the padding of NumAttrSets so hopefully this will not waste any memory.

Thanks for working on this. Making sure we can extend attributes is a bit of a hassle now that we reached the magic number ;)

This looks fine to me but I'm not to involved in these parts. Let's see if someone else want to approve this.


We are going to add a function attribute number 64.

Out of curiosity, which one do you mean here?

It's sanitize_memtag in the child revision.
Actually, we were fine without this until your change on Jule 10 pushed our pending attribute over the limit :)

vitalybuka added inline comments.
llvm/lib/IR/AttributeImpl.h
250 ↗(On Diff #209572)

std::bitset?

It's sanitize_memtag in the child revision.
Actually, we were fine without this until your change on Jule 10 pushed our pending attribute over the limit :)

It seems it's my fault...

Macro notsmartman:

(sorry for derailing this, always wanted to try these memes they added)

llvm/lib/IR/AttributeImpl.h
250 ↗(On Diff #209572)

We also have a llvm::SmallBitVector. If we keep a fixed size encoding we should add a (static) assert so it'll break in a informative manner once the new limit is reached.

std::bitset is word-aligned at least in libc++ implementation, so it would waste 4 bytes here.
SmallBitVector has only 64 bits of inline storage.

pcc added a comment.Jul 12 2019, 2:42 PM

I would prefer to keep this the way it is with an array so that it is obvious what the size/alignment are.

It would be good to have a static assertion of some sort, though. It looks like Attribute::EndAttrKinds could be used for this purpose.

In D64663#1583672, @pcc wrote:

I would prefer to keep this the way it is with an array so that it is obvious what the size/alignment are.

It would be good to have a static assertion of some sort, though. It looks like Attribute::EndAttrKinds could be used for this purpose.

We already have that in AttributeListImpl::AttributeListImpl constructor.

pcc accepted this revision.Jul 12 2019, 2:48 PM

Ah, I missed that.

LGTM

llvm/lib/IR/AttributeImpl.h
250 ↗(On Diff #209572)

Nit: maybe do uint8_t AvailableFunctionAttrs[12] = {}; to avoid needing to put the initializer in the ctor.

This revision is now accepted and ready to land.Jul 12 2019, 2:48 PM
eugenis updated this revision to Diff 209618.Jul 12 2019, 3:22 PM

oops, found another place to update

eugenis marked an inline comment as done.Jul 12 2019, 5:23 PM

Tested by adding a bunch of fake attributes at the beginning to push the interesting ones out of 0..64 range, and also with ASan.

This revision was automatically updated to reflect the committed changes.