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

   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);

   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.

     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,

                                                 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.

                                                   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.

 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();
 

   /// 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;

llvm/lib/IR/Attributes.cpp

   return getImpl(C, AttrSets);
 }
 
+AttributeList AttributeList::get(LLVMContext &C, unsigned Index,
+                                 AttributeSet AS) {
+  if (!AS.hasAttributes())
+    return {};
+  Index = attrIdxToArrayIdx(Index);
+  SmallVector<AttributeSet, 8> AttrSets(Index + 1);
+  AttrSets[Index] = AS;
+  return getImpl(C, AttrSets);
+}
+
 AttributeList AttributeList::get(LLVMContext &C, unsigned Index,
                                  ArrayRef<Attribute::AttrKind> Kinds) {
   SmallVector<std::pair<unsigned, Attribute>, 8> Attrs;

   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 {

 }
 
 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;
 }
 

   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)

   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())

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

     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);
 

   }
 
   // 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

llvm/unittests/IR/AttributesTest.cpp

 
   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);
 }