[Attributes] Make attribute addition behavior consistent

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,
but 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.

Differential Revision: https://reviews.llvm.org/D117552

Author: nikic

llvm/include/llvm/IR/Attributes.h

@@ -1048,7 +1048,8 @@ class AttrBuilder {
       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.

llvm/lib/IR/Attributes.cpp

@@ -627,11 +627,9 @@ AttributeSet AttributeSet::addAttributes(LLVMContext &C,
   if (!AS.hasAttributes())
     return *this;
 
-  AttrBuilder B(C, AS);
-  for (const auto &I : *this)
-    B.addAttribute(I);
-
- return get(C, B);
+  AttrBuilder B(C, *this);
+  B.merge(AttrBuilder(C, AS));
+  return get(C, B);
 }
 
 AttributeSet AttributeSet::removeAttribute(LLVMContext &C,
@@ -1250,15 +1248,6 @@ AttributeList AttributeList::addAttributesAtIndex(LLVMContext &C,
   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));
@@ -1744,13 +1733,12 @@ AttrBuilder &AttrBuilder::addInAllocaAttr(Type *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;

llvm/test/Transforms/Inline/ret_attr_update.ll

@@ -142,7 +142,7 @@ define i8* @test5(i8* %ptr, i64 %x) {
 
 ; 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)
@@ -153,7 +153,7 @@ define internal i8* @callee6(i8* %p) alwaysinline {
 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]]
 ;

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

@@ -134,7 +134,7 @@ define noalias i8* @memalign_unknown_align(i64 %align) {
 
 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)