llvm dev - Apr 2019 - [RFC] Proposed update to convert two 64-bit attribute bitmasks to std::bitset

There are two 64-bit bitmasks maintained in
AttributeImpl.h<https://sdocc.itg.ti.com/ui#file:review=11893/version=393846>:

- AvailableFunctionAttrs is part of the AttributeListImpl class, and
- AvailableAttrs is part of the AttributeSetNode class

Both of these assume that the number of available enum attributes is limited to
64. In fact, a static_assert in
Attributes.cpp<https://sdocc.itg.ti.com/ui#file:review=11893/version=393848>
enforces that the number of enum attributes stays at 64 or below. However, the
bitcode writer and reader don't communicate enum attributes via bitmask
anymore. Enum attributes are encoded in attribute groups.

The AvailableFunctionAttrs and AvailableAttrs bitmasks are leftovers that need
to be updated to remove the limitation on the number of enum attributes that can
be defined in the Attribute::AttrKind enum.

Per a suggestion that I received a while ago from Reid Kleckner (on the llvm-dev
list), I propose to implement both of these data members as std::bitset objects.

Here are the details for this proposed change:

llvm/lib/IR/AttributeImpl.h<https://sdocc.itg.ti.com/ui#file:review=11893/version=393846>:
- Define AvailableAttrs and AvailableFunctionAttrs with type
std::bitset<AttributeEndAttrKinds> instead of uint64_t
- Update AttributeSetNode::hasAttribute() to use the std::bitset test function
on AvailableAttrs to check if an enum attribute is present
- Update AttributeListImpl::hasFnAttribute() to use the std::bitset test
function on AvailableFunctionAttrs to check if an enum attribute is present

llvm/lib/IR/Attributes.cpp<https://sdocc.itg.ti.com/ui#file:review=11893/version=393848>:
- Update AttributeSetNode::AttributeSetNode() constructor to use the
std::bitset's set function to initialize AvailableAttrs
- Update AttributeListImpl::AttributeListImpl() constructor to use the
std::bitset's set function to initialize AvailableFunctionAttrs and remove
the static_assert that enforces an upper limit of 64 on the number of enum
attributes allowed
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190404/67ecb08a/attachment.html>

The representation change seems fine per se, but we still have a concern
with the 65th attribute increasing the size of the objects in question. 
Do you have a plan to minimize that impact?

To be clear, I think the representation change is fine regardless.  As
one example, we have a bunch of downstream attributes, and would happily
pay the extra word to integrate them cleanly with the built in variety
in our downstream code.  I'm sure we're not the only ones.

Philip

On 4/4/19 3:38 PM, Snider, Todd via llvm-dev wrote:
>
>  
>
> There are two 64-bit bitmasks maintained in AttributeImpl.h
> <https://sdocc.itg.ti.com/ui#file:review=11893/version=393846>:
>
> - AvailableFunctionAttrs is part of the AttributeListImpl class, and
> - AvailableAttrs is part of the AttributeSetNode class
>
> Both of these assume that the number of available enum attributes is
> limited to 64. In fact, a static_assert in Attributes.cpp
>
<https://sdocc.itg.ti.com/ui#file:review=11893/version=393848> enforces
> that the number of enum attributes stays at 64 or below. However, the
> bitcode writer and reader don't communicate enum attributes via
> bitmask anymore. Enum attributes are encoded in attribute groups.
>
> The AvailableFunctionAttrs and AvailableAttrs bitmasks are leftovers
> that need to be updated to remove the limitation on the number of enum
> attributes that can be defined in the Attribute::AttrKind enum.
>
> Per a suggestion that I received a while ago from Reid Kleckner (on
> the llvm-dev list), I propose to implement both of these data members
> as std::bitset objects.
>
> Here are the details for this proposed change:
>
> llvm/lib/IR/AttributeImpl.h
> <https://sdocc.itg.ti.com/ui#file:review=11893/version=393846>:
> - Define AvailableAttrs and AvailableFunctionAttrs with type
> std::bitset<AttributeEndAttrKinds> instead of uint64_t
> - Update AttributeSetNode::hasAttribute() to use the std::bitset test
> function on AvailableAttrs to check if an enum attribute is present
> - Update AttributeListImpl::hasFnAttribute() to use the std::bitset
> test function on AvailableFunctionAttrs to check if an enum attribute
> is present
>
> llvm/lib/IR/Attributes.cpp
> <https://sdocc.itg.ti.com/ui#file:review=11893/version=393848>:
> - Update AttributeSetNode::AttributeSetNode() constructor to use the
> std::bitset's set function to initialize AvailableAttrs
> - Update AttributeListImpl::AttributeListImpl() constructor to use the
> std::bitset's set function to initialize AvailableFunctionAttrs and
> remove the static_assert that enforces an upper limit of 64 on the
> number of enum attributes allowed
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190405/95c0b654/attachment.html>

Hi Philip,

I'm not sure I understand your concerns exactly.

My proposal is to construct AvailableFunctionAttrs, for example, with an initial
size that comprehends the upper limit of the number of enum attribute slots in
Attribute:AttrKind ...

class AttributeListImpl final
    : public FoldingSetNode,
      private TrailingObjects<AttributeListImpl, AttributeSet> {
  friend class AttributeList;
  friend TrailingObjects;

private:
   ...
  /// Bitset with a bit for each available attribute Attribute::AttrKind.
  std::bitset<Attribute::EndAttrKinds> AvailableFunctionAttrs;

   ...
};

Yes, I suspect that the footprint of a bitset implementation is bigger than 1 or
2 uint64_t words, but I'm wary of dictating an arbitrary limit on the number
of enum attributes allowed based on how we implement the API for keeping track
of whether a function or data object has an attribute or not.

If I'm misunderstanding what you mean by "pay[ing] an extra word to
integrate them cleanly," then can you please clarify?

~ Todd

From: Philip Reames [mailto:listmail at philipreames.com]
Sent: Friday, April 5, 2019 11:45 AM
To: Snider, Todd; llvm-dev
Subject: [EXTERNAL] Re: [llvm-dev] [RFC] Proposed update to convert two 64-bit
attribute bitmasks to std::bitset


The representation change seems fine per se, but we still have a concern with
the 65th attribute increasing the size of the objects in question.  Do you have
a plan to minimize that impact?

To be clear, I think the representation change is fine regardless.  As one
example, we have a bunch of downstream attributes, and would happily pay the
extra word to integrate them cleanly with the built in variety in our downstream
code.  I'm sure we're not the only ones.

Philip
On 4/4/19 3:38 PM, Snider, Todd via llvm-dev wrote:

There are two 64-bit bitmasks maintained in
AttributeImpl.h<https://sdocc.itg.ti.com/ui#file:review=11893/version=393846>:

- AvailableFunctionAttrs is part of the AttributeListImpl class, and
- AvailableAttrs is part of the AttributeSetNode class

Both of these assume that the number of available enum attributes is limited to
64. In fact, a static_assert in
Attributes.cpp<https://sdocc.itg.ti.com/ui#file:review=11893/version=393848>
enforces that the number of enum attributes stays at 64 or below. However, the
bitcode writer and reader don't communicate enum attributes via bitmask
anymore. Enum attributes are encoded in attribute groups.

The AvailableFunctionAttrs and AvailableAttrs bitmasks are leftovers that need
to be updated to remove the limitation on the number of enum attributes that can
be defined in the Attribute::AttrKind enum.

Per a suggestion that I received a while ago from Reid Kleckner (on the llvm-dev
list), I propose to implement both of these data members as std::bitset objects.

Here are the details for this proposed change:

llvm/lib/IR/AttributeImpl.h<https://sdocc.itg.ti.com/ui#file:review=11893/version=393846>:
- Define AvailableAttrs and AvailableFunctionAttrs with type
std::bitset<AttributeEndAttrKinds> instead of uint64_t
- Update AttributeSetNode::hasAttribute() to use the std::bitset test function
on AvailableAttrs to check if an enum attribute is present
- Update AttributeListImpl::hasFnAttribute() to use the std::bitset test
function on AvailableFunctionAttrs to check if an enum attribute is present

llvm/lib/IR/Attributes.cpp<https://sdocc.itg.ti.com/ui#file:review=11893/version=393848>:
- Update AttributeSetNode::AttributeSetNode() constructor to use the
std::bitset's set function to initialize AvailableAttrs
- Update AttributeListImpl::AttributeListImpl() constructor to use the
std::bitset's set function to initialize AvailableFunctionAttrs and remove
the static_assert that enforces an upper limit of 64 on the number of enum
attributes allowed



_______________________________________________

LLVM Developers mailing list

llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>

https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190405/707cf05f/attachment.html>