[llvm] da61cb0 - [Attributes] Make attribute addition behavior consistent
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 19 03:05:38 PST 2022
Author: Nikita Popov
Date: 2022-01-19T12:05:27+01:00
New Revision: da61cb019eb2aa624862212960be3609a744b26e
URL: https://github.com/llvm/llvm-project/commit/da61cb019eb2aa624862212960be3609a744b26e
DIFF: https://github.com/llvm/llvm-project/commit/da61cb019eb2aa624862212960be3609a744b26e.diff
LOG: [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
Added:
Modified:
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
Removed:
################################################################################
diff --git a/llvm/include/llvm/IR/Attributes.h b/llvm/include/llvm/IR/Attributes.h
index de559ce3cf172..9f3ed27c7a920 100644
--- a/llvm/include/llvm/IR/Attributes.h
+++ b/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.
diff --git a/llvm/lib/IR/Attributes.cpp b/llvm/lib/IR/Attributes.cpp
index e9055d3f724a4..6220a39a9a785 100644
--- a/llvm/lib/IR/Attributes.cpp
+++ b/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;
diff --git a/llvm/test/Transforms/Inline/ret_attr_update.ll b/llvm/test/Transforms/Inline/ret_attr_update.ll
index bb10920ebf0ea..a6ab988b24468 100644
--- a/llvm/test/Transforms/Inline/ret_attr_update.ll
+++ b/llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -142,7 +142,7 @@ define i8* @test5(i8* %ptr, i64 %x) {
; deref attributes have
diff erent 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]]
;
diff --git a/llvm/test/Transforms/InstCombine/deref-alloc-fns.ll b/llvm/test/Transforms/InstCombine/deref-alloc-fns.ll
index 3adfab914175f..50421cc667967 100644
--- a/llvm/test/Transforms/InstCombine/deref-alloc-fns.ll
+++ b/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)
More information about the llvm-commits
mailing list