Avoid useless AttrBuilder conversion

[serge-sans-paille]

Summary

AttrBuilder has two constructors that allow for costly implict conversion, when ad-hoc method could do the job more efficiently.
Make these constructors explicit, and provide the ad-hoc specialized method.

[serge-sans-paille]

Performance impact: https://llvm-compile-time-tracker.com/compare.php?from=624f12d34f94a9e814753f26f0b499870697b68c&to=39593244ec276f0d9fd53db9e178511fe455ed88&stat=instructions

[serge-sans-paille]

(rebased on main branch)

[nikic]

llvm/include/llvm/IR/Attributes.h

455

Duplicate declaration with a few lines below, due to a similar change I landed earlier today (https://github.com/llvm/llvm-project/commit/6e30cb7673df293fd294acef7eadca8050b5a71e).

490

I'm not sure I understand the benefit of this method, and similar ones below. In the end, won't we still convert this AttributeSet into an AttrBuilder for merging?

I think the only case where this could make a difference is if the Index is currently unused, in which case the original attribute set can be directly used. Is this the case being optimized?

[serge-sans-paille]

llvm/lib/IR/Attributes.cpp

1286

@nikic As can be seen here, we're not converting AS to an AttrBuilder, directly calling the merge from an AttributeSet

1793

This merge doesn't require a temporary AttrBuilder

[serge-sans-paille]

llvm/include/llvm/IR/Attributes.h

455

gotcha

[serge-sans-paille]

Minor nits

llvm/include/llvm/IR/Attributes.h

public:
   //===--------------------------------------------------------------------===//
   // AttributeList Construction and Mutation
   //===--------------------------------------------------------------------===//
 
   /// Return an AttributeList with the specified parameters in it.
   static AttributeList get(LLVMContext &C, ArrayRef<AttributeList> Attrs);
   static AttributeList get(LLVMContext &C, unsigned Index,
                            ArrayRef<Attribute::AttrKind> Kinds);
   static AttributeList get(LLVMContext &C, unsigned Index,
+                           AttributeSet AS);
+  static AttributeList get(LLVMContext &C, unsigned Index,
                            ArrayRef<Attribute::AttrKind> Kinds,
                            ArrayRef<uint64_t> Values);
   static AttributeList get(LLVMContext &C, unsigned Index,
                            ArrayRef<StringRef> Kind);
   static AttributeList get(LLVMContext &C, unsigned Index,
                            AttributeSet Attrs);
   static AttributeList get(LLVMContext &C, unsigned Index,
                            const AttrBuilder &B);
LLVM_NODISCARD AttributeList addAttributeAtIndex(LLVMContext &C,
                                                    unsigned Index,
                                                    Attribute A) const;
 
   /// Add attributes to the attribute set at the given index.
   /// Returns a new list because attribute lists are immutable.
   LLVM_NODISCARD AttributeList addAttributesAtIndex(LLVMContext &C,
                                                     unsigned Index,
                                                     const AttrBuilder &B) const;
+  LLVM_NODISCARD AttributeList addAttributesAtIndex(LLVMContext &C,
+                                                    unsigned Index,
+                                                    AttributeSet AS) const;
 
   /// Add a function attribute to the list. Returns a new list because
   /// attribute lists are immutable.
   LLVM_NODISCARD AttributeList addFnAttribute(LLVMContext &C,
                                               Attribute::AttrKind Kind) const {
     return addAttributeAtIndex(C, FunctionIndex, Kind);
   }
 
public:
 
   /// Add function attribute to the list. Returns a new list because
   /// attribute lists are immutable.
   LLVM_NODISCARD AttributeList addFnAttributes(LLVMContext &C,
                                                const AttrBuilder &B) const {
     return addAttributesAtIndex(C, FunctionIndex, B);
   }
 
+  LLVM_NODISCARD AttributeList addFnAttributes(LLVMContext &C,
+                                               AttributeSet AS) const {
+    return addAttributesAtIndex(C, FunctionIndex, AS);
+  }
+
   /// Add a return value attribute to the list. Returns a new list because
   /// attribute lists are immutable.
   LLVM_NODISCARD AttributeList addRetAttribute(LLVMContext &C,
                                                Attribute::AttrKind Kind) const {
     return addAttributeAtIndex(C, ReturnIndex, Kind);
   }
 
   /// Add a return value attribute to the list. Returns a new list because
   /// attribute lists are immutable.
   LLVM_NODISCARD AttributeList addRetAttribute(LLVMContext &C,
                                                Attribute Attr) const {
     return addAttributeAtIndex(C, ReturnIndex, Attr);
   }
 
   /// Add a return value attribute to the list. Returns a new list because
   /// attribute lists are immutable.
   LLVM_NODISCARD AttributeList addRetAttributes(LLVMContext &C,
                                                 const AttrBuilder &B) const {
     return addAttributesAtIndex(C, ReturnIndex, B);
   }
+  LLVM_NODISCARD AttributeList addRetAttributes(LLVMContext &C,
+                                                AttributeSet AS) const {
+    return addAttributesAtIndex(C, ReturnIndex, AS);
+  }
 
   /// Add an argument attribute to the list. Returns a new list because
   /// attribute lists are immutable.
   LLVM_NODISCARD AttributeList addParamAttribute(
       LLVMContext &C, unsigned ArgNo, Attribute::AttrKind Kind) const {
     return addAttributeAtIndex(C, ArgNo + FirstArgIndex, Kind);
   }
 
public:
 
   /// Add an argument attribute to the list. Returns a new list because
   /// attribute lists are immutable.
   LLVM_NODISCARD AttributeList addParamAttributes(LLVMContext &C,
                                                   unsigned ArgNo,
                                                   const AttrBuilder &B) const {
     return addAttributesAtIndex(C, ArgNo + FirstArgIndex, B);
   }
+  LLVM_NODISCARD AttributeList addParamAttributes(LLVMContext &C,
+                                                  unsigned ArgNo,
+                                                  AttributeSet AS) const {
+    return addAttributesAtIndex(C, ArgNo + FirstArgIndex, AS);
+  }
 
   /// Remove the specified attribute at the specified index from this
   /// attribute list. Returns a new list because attribute lists are immutable.
   LLVM_NODISCARD AttributeList removeAttributeAtIndex(
       LLVMContext &C, unsigned Index, Attribute::AttrKind Kind) const;
 
   /// Remove the specified attribute at the specified index from this
   /// attribute list. Returns a new list because attribute lists are immutable.
class AttrBuilder {
   std::array<Type *, Attribute::NumTypeAttrKinds> TypeAttrs = {};
 
   Optional<unsigned> kindToIntIndex(Attribute::AttrKind Kind) const;
   Optional<unsigned> kindToTypeIndex(Attribute::AttrKind Kind) const;
 
 public:
   AttrBuilder() = default;
 
-  AttrBuilder(const Attribute &A) {
+  explicit AttrBuilder(const Attribute &A) {
     addAttribute(A);
   }
 
+  explicit AttrBuilder(AttributeSet AS);
+
   AttrBuilder(AttributeList AS, unsigned Idx);
-  AttrBuilder(AttributeSet AS);
 
   void clear();
 
   /// Add an attribute to the builder.
   AttrBuilder &addAttribute(Attribute::AttrKind Val) {
     assert((unsigned)Val < Attribute::EndAttrKinds &&
            "Attribute out of range!");
     assert(Attribute::isEnumAttrKind(Val) &&
public:
   AttrBuilder &addAttribute(StringRef A, StringRef V = StringRef());
 
   /// Remove an attribute from the builder.
   AttrBuilder &removeAttribute(Attribute::AttrKind Val);
 
   /// Remove the attributes from the builder.
   AttrBuilder &removeAttributes(AttributeList A, uint64_t WithoutIndex);
 
+  /// Remove the attributes from the builder.
+  AttrBuilder &removeAttributes(AttributeSet AS);
+
   /// Remove the target-dependent attribute to the builder.
   AttrBuilder &removeAttribute(StringRef A);
 
   /// Add the attributes from the builder.
   AttrBuilder &merge(const AttrBuilder &B);
 
+  /// Add the attributes from the builder.
+  AttrBuilder &merge(AttributeSet AS);
+
   /// Remove the attributes from the builder.
   AttrBuilder &remove(const AttrBuilder &B);
 
+  /// Remove the attribute from the builder.
+  AttrBuilder &remove(Attribute A);
+
   /// Return true if the builder has any attribute that's in the
   /// specified builder.
   bool overlaps(const AttrBuilder &B) const;
 
   /// Return true if the builder has the specified attribute.
   bool contains(Attribute::AttrKind A) const {
     assert((unsigned)A < Attribute::EndAttrKinds && "Attribute out of range!");
     return Attrs[A];

llvm/lib/IR/Attributes.cpp

assert((!OldAlign || !NewAlign || OldAlign == NewAlign) &&
          "Attempt to change alignment!");
 #endif
 
   AttrBuilder Merged(getAttributes(Index));
   Merged.merge(B);
   return setAttributesAtIndex(C, Index, AttributeSet::get(C, Merged));
 }
 
+AttributeList AttributeList::addAttributesAtIndex(LLVMContext &C,
+                                                  unsigned Index,
+                                                  AttributeSet AS) const {
+  if (!AS.hasAttributes())
+    return *this;
+
+  if (!pImpl)
+    return AttributeList::get(C, {{Index, AS}});
+
+#ifndef NDEBUG
+  // FIXME it is not obvious how this should work for alignment. For now, say
+  // we can't change a known alignment.
+  const MaybeAlign OldAlign = getAttributes(Index).getAlignment();
+  const MaybeAlign NewAlign = AS.getAlignment();
+  assert((!OldAlign || !NewAlign || OldAlign == NewAlign) &&
+         "Attempt to change alignment!");
+#endif
+
+  AttrBuilder Merged(getAttributes(Index));
+  Merged.merge(AS);
+  return setAttributesAtIndex(C, Index, AttributeSet::get(C, Merged));
+}
+
 AttributeList AttributeList::addParamAttribute(LLVMContext &C,
                                                ArrayRef<unsigned> ArgNos,
                                                Attribute A) const {
   assert(llvm::is_sorted(ArgNos));
 
   SmallVector<AttributeSet, 4> AttrSets(this->begin(), this->end());
   unsigned MaxIndex = attrIdxToArrayIdx(ArgNos.back() + FirstArgIndex);
   if (MaxIndex >= AttrSets.size())
if (Optional<unsigned> TypeIndex = kindToTypeIndex(Val))
     TypeAttrs[*TypeIndex] = nullptr;
   else if (Optional<unsigned> IntIndex = kindToIntIndex(Val))
     IntAttrs[*IntIndex] = 0;
 
   return *this;
 }
 
 AttrBuilder &AttrBuilder::removeAttributes(AttributeList A, uint64_t Index) {
-  remove(A.getAttributes(Index));
+  removeAttributes(A.getAttributes(Index));
+  return *this;
+}
+
+AttrBuilder &AttrBuilder::removeAttributes(AttributeSet AS) {
+  for(Attribute A: AS)
+    remove(A);
   return *this;
 }
 
 AttrBuilder &AttrBuilder::removeAttribute(StringRef A) {
   TargetDepAttrs.erase(A);
   return *this;
 }
 
AttrBuilder &AttrBuilder::merge(const AttrBuilder &B) {
   Attrs |= B.Attrs;
 
   for (const auto &I : B.td_attrs())
     TargetDepAttrs[I.first] = I.second;
 
   return *this;
 }
 
+AttrBuilder &AttrBuilder::merge(AttributeSet AS) {
+  for(Attribute A : AS)
+    addAttribute(A);
+  return *this;
+}
+
 AttrBuilder &AttrBuilder::remove(const AttrBuilder &B) {
   // FIXME: What if both have an int/type attribute, but they don't match?!
   for (unsigned Index = 0; Index < Attribute::NumIntAttrKinds; ++Index)
     if (B.IntAttrs[Index])
       IntAttrs[Index] = 0;
 
   for (unsigned Index = 0; Index < Attribute::NumTypeAttrKinds; ++Index)
     if (B.TypeAttrs[Index])
       TypeAttrs[Index] = nullptr;
 
   Attrs &= ~B.Attrs;
 
   for (const auto &I : B.td_attrs())
     TargetDepAttrs.erase(I.first);
 
   return *this;
 }
 
+AttrBuilder &AttrBuilder::remove(Attribute A) {
+  if(A.isStringAttribute())
+    return removeAttribute(A.getKindAsString());
+  else
+    return removeAttribute(A.getKindAsEnum());
+}
+
 bool AttrBuilder::overlaps(const AttrBuilder &B) const {
   // First check if any of the target independent attributes overlap.
   if ((Attrs & B.Attrs).any())
     return true;
 
   // Then check if any target dependent ones do.
   for (const auto &I : td_attrs())
     if (B.contains(I.first))

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

 // Create new attribute set containing only attributes which can be transferred
 // from original call to the safepoint.
 static AttributeList legalizeCallAttributes(LLVMContext &Ctx,
                                             AttributeList AL) {
   if (AL.isEmpty())
     return AL;
 
   // Remove the readonly, readnone, and statepoint function attributes.
-  AttrBuilder FnAttrs = AL.getFnAttrs();
+  AttrBuilder FnAttrs(AL.getFnAttrs());
   for (auto Attr : FnAttrsToStrip)
     FnAttrs.removeAttribute(Attr);
 
   for (Attribute A : AL.getFnAttrs()) {
     if (isStatepointDirectiveAttr(A))
       FnAttrs.remove(A);
   }
 
   // Just skip parameter and return attributes for now
-  return AttributeList::get(Ctx, AttributeList::FunctionIndex,
-                            AttributeSet::get(Ctx, FnAttrs));
+  return AttributeList::get(Ctx, AttributeList::FunctionIndex, FnAttrs);
 }
 
 /// Helper function to place all gc relocates necessary for the given
 /// statepoint.
 /// Inputs:
 ///   liveVariables - list of variables to be relocated.
 ///   basePtrs - base pointers.
 ///   statepointToken - statepoint instruction to which relocates should be

llvm/unittests/IR/AttributesTest.cpp

TEST(Attributes, Ordering) {
   EXPECT_TRUE(ByVal < Align4);
   EXPECT_FALSE(ByVal < ByVal);
 
   AttributeList ASs[] = {AttributeList::get(C, 2, Attribute::ZExt),
                          AttributeList::get(C, 1, Attribute::SExt)};
 
   AttributeList SetA = AttributeList::get(C, ASs);
   AttributeList SetB =
-      SetA.removeParamAttributes(C, 0, ASs[1].getParamAttrs(0));
+      SetA.removeParamAttributes(C, 0, AttrBuilder(ASs[1].getParamAttrs(0)));
   EXPECT_NE(SetA, SetB);
 }
 
 TEST(Attributes, AddAttributes) {
   LLVMContext C;
   AttributeList AL;
   AttrBuilder B;
   B.addAttribute(Attribute::NoReturn);