[Attributes] Make attribute addition behavior consistent

[nikic]

Summary

Currently, the behavior when adding an attribute with the same key as an existing attribute is inconsistent, depending on the type of the attribute and the method used to add it. When going through AttrBuilder::addAttribute(), the new attribute always overwrites the old one. When going through AttrBuilder::merge() the new attribute overwrites the existing one if it is a string attribute, not keeps the existing one for int and type attributes. One particular API also asserts that you can't overwrite an align attribute, but does not handle any of the other int, type or string attributes.

This patch makes the behavior consistent by always overwriting with the new attribute, which is the behavior I would intuitively expect. Two tests are affected, which now make a different (but equally valid) choice. Those tests could be improved by taking the maximum deref bytes, but I haven't bothered with that, since this is testing a degenerate case -- the important bit is that it doesn't crash.

The main alternative I see is to instead assert that you cannot add an attribute that already exists, unless it has the same value, forcing the user to handle this explicitly by removing the old attribute first.

[serge-sans-paille]

llvm/lib/IR/Attributes.cpp

1737

It seems to me this could just be IntAttrs |= B.IntAttrs which is probably faster.

[nikic]

llvm/lib/IR/Attributes.cpp

1737

I don't think that std::array has an operator|= overload, and it probably wouldn't do what we want here (if it just works elementwise): For example, for dereferenceable(1) and deferenceable(2) we don't want to get back dereferenceable(3).

llvm/include/llvm/IR/Attributes.h

public:
   /// Remove the target-dependent attribute from the builder.
   AttrBuilder &removeAttribute(Attribute A) {
     if (A.isStringAttribute())
       return removeAttribute(A.getKindAsString());
     else
       return removeAttribute(A.getKindAsEnum());
   }
 
-  /// Add the attributes from the builder.
+  /// Add the attributes from the builder. Attributes in the passed builder
+  /// overwrite attributes in this builder if they have the same key.
   AttrBuilder &merge(const AttrBuilder &B);
 
   /// Remove the attributes from the builder.
   AttrBuilder &remove(const AttributeMask &AM);
 
   /// Return true if the builder has any attribute that's in the
   /// specified builder.
   bool overlaps(const AttributeMask &AM) const;

llvm/lib/IR/Attributes.cpp

 AttributeSet AttributeSet::addAttributes(LLVMContext &C,
                                          const AttributeSet AS) const {
   if (!hasAttributes())
     return AS;
 
   if (!AS.hasAttributes())
     return *this;
 
-  AttrBuilder B(C, AS);
-  for (const auto &I : *this)
-    B.addAttribute(I);
-
+  AttrBuilder B(C, *this);
+  B.merge(AttrBuilder(C, AS));
  return get(C, B);
 }
 
 AttributeSet AttributeSet::removeAttribute(LLVMContext &C,
                                              Attribute::AttrKind Kind) const {
   if (!hasAttribute(Kind)) return *this;
   AttrBuilder B(C, *this);
   B.removeAttribute(Kind);
   return get(C, B);
AttributeList AttributeList::addAttributesAtIndex(LLVMContext &C,
                                                   unsigned Index,
                                                   const AttrBuilder &B) const {
   if (!B.hasAttributes())
     return *this;
 
   if (!pImpl)
     return AttributeList::get(C, {{Index, AttributeSet::get(C, B)}});
 
-#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 = B.getAlignment();
-  assert((!OldAlign || !NewAlign || OldAlign == NewAlign) &&
-         "Attempt to change alignment!");
-#endif
-
   AttrBuilder Merged(C, getAttributes(Index));
   Merged.merge(B);
   return setAttributesAtIndex(C, Index, AttributeSet::get(C, Merged));
 }
 
 AttributeList AttributeList::addParamAttribute(LLVMContext &C,
                                                ArrayRef<unsigned> ArgNos,
                                                Attribute A) const {
AttrBuilder &AttrBuilder::addPreallocatedAttr(Type *Ty) {
   return addTypeAttr(Attribute::Preallocated, Ty);
 }
 
 AttrBuilder &AttrBuilder::addInAllocaAttr(Type *Ty) {
   return addTypeAttr(Attribute::InAlloca, Ty);
 }
 
 AttrBuilder &AttrBuilder::merge(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 (!IntAttrs[Index])
+    if (B.IntAttrs[Index])
       IntAttrs[Index] = B.IntAttrs[Index];
 
   for (unsigned Index = 0; Index < Attribute::NumTypeAttrKinds; ++Index)
-    if (!TypeAttrs[Index])
+    if (B.TypeAttrs[Index])
       TypeAttrs[Index] = B.TypeAttrs[Index];
 
   Attrs |= B.Attrs;
 
   // TODO: could merge both lists in one loop
   for (const auto &I : B.td_attrs())
     addAttribute(I);
 

llvm/test/Transforms/Inline/ret_attr_update.ll

 ;
   %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
   %s = call dereferenceable_or_null(12) i8* @callee5(i8* %gep)
   ret i8* %s
 }
 
 ; deref attributes have different values on the callee and the call feeding into
 ; the return.
-; AttrBuilder chooses the already existing value and does not overwrite it.
+; AttrBuilder overwrites the existing value.
 define internal i8* @callee6(i8* %p) alwaysinline {
   %r = call dereferenceable_or_null(16) i8* @foo(i8* %p)
   %v = call i8* @baz(i8* %p)
   ret i8* %r
 }
 
 
 define i8* @test6(i8* %ptr, i64 %x) {
 ; CHECK-LABEL: @test6(
 ; CHECK-NEXT:    [[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
-; CHECK-NEXT:    [[R_I:%.*]] = call dereferenceable_or_null(16) i8* @foo(i8* [[GEP]])
+; CHECK-NEXT:    [[R_I:%.*]] = call dereferenceable_or_null(12) i8* @foo(i8* [[GEP]])
 ; CHECK-NEXT:    [[V_I:%.*]] = call i8* @baz(i8* [[GEP]])
 ; CHECK-NEXT:    ret i8* [[R_I]]
 ;
   %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
   %s = call dereferenceable_or_null(12) i8* @callee6(i8* %gep)
   ret i8* %s
 }
 

llvm/test/Transforms/InstCombine/deref-alloc-fns.ll

 ; GNU-NEXT:    ret i8* [[CALL]]
 ;
   %call = tail call noalias i8* @memalign(i64 %align, i64 128)
   ret i8* %call
 }
 
 define noalias i8* @malloc_constant_size2() {
 ; CHECK-LABEL: @malloc_constant_size2(
-; CHECK-NEXT:    [[CALL:%.*]] = tail call noalias dereferenceable_or_null(80) i8* @malloc(i64 40)
+; CHECK-NEXT:    [[CALL:%.*]] = tail call noalias dereferenceable_or_null(40) i8* @malloc(i64 40)
 ; CHECK-NEXT:    ret i8* [[CALL]]
 ;
   %call = tail call noalias dereferenceable_or_null(80) i8* @malloc(i64 40)
   ret i8* %call
 }
 
 define noalias i8* @malloc_constant_size3() {
 ; CHECK-LABEL: @malloc_constant_size3(