[PATCH] D117552: [Attributes] Make attribute addition behavior consistent

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: serge-sans-paille, rnk, aeubanks.
Herald added subscribers: dexonsmith, jdoerfert, hiraditya.
nikic requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

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.


https://reviews.llvm.org/D117552

Files:
  llvm/include/llvm/IR/Attributes.h
  llvm/lib/IR/Attributes.cpp
  llvm/test/Transforms/Inline/ret_attr_update.ll
  llvm/test/Transforms/InstCombine/deref-alloc-fns.ll


Index: llvm/test/Transforms/InstCombine/deref-alloc-fns.ll
===================================================================
--- llvm/test/Transforms/InstCombine/deref-alloc-fns.ll
+++ llvm/test/Transforms/InstCombine/deref-alloc-fns.ll
@@ -134,7 +134,7 @@
 
 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)
Index: llvm/test/Transforms/Inline/ret_attr_update.ll
===================================================================
--- llvm/test/Transforms/Inline/ret_attr_update.ll
+++ llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -142,7 +142,7 @@
 
 ; 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 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]]
 ;
Index: llvm/lib/IR/Attributes.cpp
===================================================================
--- llvm/lib/IR/Attributes.cpp
+++ llvm/lib/IR/Attributes.cpp
@@ -627,10 +627,8 @@
   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);
 }
 
@@ -1250,15 +1248,6 @@
   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::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;
Index: llvm/include/llvm/IR/Attributes.h
===================================================================
--- llvm/include/llvm/IR/Attributes.h
+++ llvm/include/llvm/IR/Attributes.h
@@ -1048,7 +1048,8 @@
       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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D117552.400785.patch
Type: text/x-patch
Size: 3818 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220118/14f8b856/attachment.bin>