[IR] Make dead_on_return attribute sized #171712
Conversation
Created using spr 1.3.7
This patch makes the dead_on_return parameter attribute require a number
of bytes to be passed in to specify the number of bytes known to be dead
upon function return/unwind. This is aimed at enabling annotating the
this pointer in C++ destructors with dead_on_return in clang. We need
this to handle cases like the following:
struct X {
int n;
~X() {
this[n].n = 0;
}
};
void f() {
X xs[] = {42, -1};
}
Where we only certain that sizeof(X) bytes are dead upon return of ~X.
Otherwise DSE would be able to eliminate the store in ~X which would not
be correct.
This patch only does the wiring within IR. Future patches will make
clang emit correct sizing information and update DSE to only delete
stores to objects marked dead_on_return that are provably in bounds of
the number of bytes specified to be dead_on_return.
Pull Request: llvm#171712
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-transforms Author: Aiden Grossman (boomanaiden154) ChangesThis patch makes the dead_on_return parameter attribute require a number Where we only certain that sizeof(X) bytes are dead upon return of ~X. This patch only does the wiring within IR. Future patches will make Full diff: https://github.com/llvm/llvm-project/pull/171712.diff 16 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 5fa3a4ebb2472..069c8e3f79657 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -1842,11 +1842,13 @@ Currently, only the following parameter attributes are defined:
This attribute cannot be applied to return values.
-``dead_on_return``
+``dead_on_return(<n>)``
This attribute indicates that the memory pointed to by the argument is dead
upon function return, both upon normal return and if the calls unwinds, meaning
that the caller will not depend on its contents. Stores that would be observable
- either on the return path or on the unwind path may be elided.
+ either on the return path or on the unwind path may be elided. The number of
+ bytes known to be dead must be provided in parentheses. It is legal for the
+ number of bytes to be less than the size of the pointee type.
Specifically, the behavior is as-if any memory written through the pointer
during the execution of the function is overwritten with a poison value
diff --git a/llvm/include/llvm/IR/Argument.h b/llvm/include/llvm/IR/Argument.h
index 6ffc0f8fd5155..df380919629fe 100644
--- a/llvm/include/llvm/IR/Argument.h
+++ b/llvm/include/llvm/IR/Argument.h
@@ -79,7 +79,7 @@ class Argument final : public Value {
LLVM_ABI bool hasByValAttr() const;
/// Return true if this argument has the dead_on_return attribute.
- LLVM_ABI bool hasDeadOnReturnAttr() const;
+ LLVM_ABI uint64_t getDeadOnReturnBytes() const;
/// Return true if this argument has the byref attribute.
LLVM_ABI bool hasByRefAttr() const;
diff --git a/llvm/include/llvm/IR/Attributes.h b/llvm/include/llvm/IR/Attributes.h
index e734466ce20e0..241c5060ec6cf 100644
--- a/llvm/include/llvm/IR/Attributes.h
+++ b/llvm/include/llvm/IR/Attributes.h
@@ -276,6 +276,10 @@ class Attribute {
/// dereferenceable attribute.
LLVM_ABI uint64_t getDereferenceableBytes() const;
+ /// Returns the number of dead_on_return bytes from the
+ /// dead_on_return attribute.
+ LLVM_ABI uint64_t getDeadOnReturnBytes() const;
+
/// Returns the number of dereferenceable_or_null bytes from the
/// dereferenceable_or_null attribute.
LLVM_ABI uint64_t getDereferenceableOrNullBytes() const;
@@ -445,6 +449,7 @@ class AttributeSet {
LLVM_ABI MaybeAlign getAlignment() const;
LLVM_ABI MaybeAlign getStackAlignment() const;
LLVM_ABI uint64_t getDereferenceableBytes() const;
+ LLVM_ABI uint64_t getDeadOnReturnBytes() const;
LLVM_ABI uint64_t getDereferenceableOrNullBytes() const;
LLVM_ABI Type *getByValType() const;
LLVM_ABI Type *getStructRetType() const;
@@ -964,6 +969,9 @@ class AttributeList {
/// the return value.
LLVM_ABI uint64_t getRetDereferenceableOrNullBytes() const;
+ /// Get the number of dead_on_return bytes (or zero if unknown) of an arg.
+ LLVM_ABI uint64_t getDeadOnReturnBytes(unsigned Index) const;
+
/// Get the number of dereferenceable_or_null bytes (or zero if unknown) of an
/// arg.
LLVM_ABI uint64_t getParamDereferenceableOrNullBytes(unsigned ArgNo) const;
@@ -1242,6 +1250,10 @@ class AttrBuilder {
/// internally in Attribute.
LLVM_ABI AttrBuilder &addDereferenceableAttr(uint64_t Bytes);
+ /// This turns the number of dead_on_return bytes into the form used
+ /// internally in Attribute.
+ LLVM_ABI AttrBuilder &addDeadOnReturnAttr(uint64_t Bytes);
+
/// This turns the number of dereferenceable_or_null bytes into the
/// form used internally in Attribute.
LLVM_ABI AttrBuilder &addDereferenceableOrNullAttr(uint64_t Bytes);
diff --git a/llvm/include/llvm/IR/Attributes.td b/llvm/include/llvm/IR/Attributes.td
index c086a39616249..e4db0e6984780 100644
--- a/llvm/include/llvm/IR/Attributes.td
+++ b/llvm/include/llvm/IR/Attributes.td
@@ -204,7 +204,7 @@ def NoFree : EnumAttr<"nofree", IntersectAnd, [FnAttr, ParamAttr]>;
def DeadOnUnwind : EnumAttr<"dead_on_unwind", IntersectAnd, [ParamAttr]>;
/// Argument is dead upon function return.
-def DeadOnReturn : EnumAttr<"dead_on_return", IntersectAnd, [ParamAttr]>;
+def DeadOnReturn : IntAttr<"dead_on_return", IntersectMin, [ParamAttr]>;
/// Disable implicit floating point insts.
def NoImplicitFloat : EnumAttr<"noimplicitfloat", IntersectPreserve, [FnAttr]>;
diff --git a/llvm/include/llvm/IR/Function.h b/llvm/include/llvm/IR/Function.h
index d3497716ca844..48b1604bdea32 100644
--- a/llvm/include/llvm/IR/Function.h
+++ b/llvm/include/llvm/IR/Function.h
@@ -523,6 +523,12 @@ class LLVM_ABI Function : public GlobalObject, public ilist_node<Function> {
return AttributeSets.getParamDereferenceableBytes(ArgNo);
}
+ /// Extract the number of dead_on_return bytes for a parameter.
+ /// @param ArgNo Index of an argument, with 0 being the first function arg.
+ uint64_t getDeadOnReturnBytes(unsigned ArgNo) const {
+ return AttributeSets.getDeadOnReturnBytes(ArgNo);
+ }
+
/// Extract the number of dereferenceable_or_null bytes for a
/// parameter.
/// @param ArgNo AttributeList ArgNo, referring to an argument.
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index a09ab4fc7828c..3600072601adc 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -1612,6 +1612,13 @@ bool LLParser::parseEnumAttribute(Attribute::AttrKind Attr, AttrBuilder &B,
B.addDereferenceableAttr(Bytes);
return false;
}
+ case Attribute::DeadOnReturn: {
+ uint64_t Bytes;
+ if (parseOptionalDerefAttrBytes(lltok::kw_dead_on_return, Bytes))
+ return true;
+ B.addDeadOnReturnAttr(Bytes);
+ return false;
+ }
case Attribute::DereferenceableOrNull: {
uint64_t Bytes;
if (parseOptionalDerefAttrBytes(lltok::kw_dereferenceable_or_null, Bytes))
@@ -2476,7 +2483,8 @@ bool LLParser::parseOptionalCodeModel(CodeModel::Model &model) {
bool LLParser::parseOptionalDerefAttrBytes(lltok::Kind AttrKind,
uint64_t &Bytes) {
assert((AttrKind == lltok::kw_dereferenceable ||
- AttrKind == lltok::kw_dereferenceable_or_null) &&
+ AttrKind == lltok::kw_dereferenceable_or_null ||
+ AttrKind == lltok::kw_dead_on_return) &&
"contract!");
Bytes = 0;
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 9dd993f0dc173..fcaa7865247d1 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -80,6 +80,7 @@
#include <cstddef>
#include <cstdint>
#include <deque>
+#include <limits>
#include <map>
#include <memory>
#include <optional>
@@ -2384,6 +2385,8 @@ Error BitcodeReader::parseAttributeGroupBlock() {
B.addInAllocaAttr(nullptr);
else if (Kind == Attribute::UWTable)
B.addUWTableAttr(UWTableKind::Default);
+ else if (Kind == Attribute::DeadOnReturn)
+ B.addDeadOnReturnAttr(std::numeric_limits<uint64_t>::max());
else if (Attribute::isEnumAttrKind(Kind))
B.addAttribute(Kind);
else
@@ -2402,6 +2405,8 @@ Error BitcodeReader::parseAttributeGroupBlock() {
B.addDereferenceableAttr(Record[++i]);
else if (Kind == Attribute::DereferenceableOrNull)
B.addDereferenceableOrNullAttr(Record[++i]);
+ else if (Kind == Attribute::DeadOnReturn)
+ B.addDeadOnReturnAttr(Record[++i]);
else if (Kind == Attribute::AllocSize)
B.addAllocSizeAttrFromRawRepr(Record[++i]);
else if (Kind == Attribute::VScaleRange)
diff --git a/llvm/lib/IR/AttributeImpl.h b/llvm/lib/IR/AttributeImpl.h
index 707c8205ee1f9..da216458d0b19 100644
--- a/llvm/lib/IR/AttributeImpl.h
+++ b/llvm/lib/IR/AttributeImpl.h
@@ -332,6 +332,7 @@ class AttributeSetNode final
MaybeAlign getAlignment() const;
MaybeAlign getStackAlignment() const;
uint64_t getDereferenceableBytes() const;
+ uint64_t getDeadOnReturnBytes() const;
uint64_t getDereferenceableOrNullBytes() const;
std::optional<std::pair<unsigned, std::optional<unsigned>>> getAllocSizeArgs()
const;
diff --git a/llvm/lib/IR/Attributes.cpp b/llvm/lib/IR/Attributes.cpp
index fe6d3e5edeb09..87d30672de956 100644
--- a/llvm/lib/IR/Attributes.cpp
+++ b/llvm/lib/IR/Attributes.cpp
@@ -451,6 +451,13 @@ uint64_t Attribute::getDereferenceableBytes() const {
return pImpl->getValueAsInt();
}
+uint64_t Attribute::getDeadOnReturnBytes() const {
+ assert(hasAttribute(Attribute::DeadOnReturn) &&
+ "Trying to get dead_on_return bytes from"
+ "from a parameter without such an attribute!");
+ return pImpl->getValueAsInt();
+}
+
uint64_t Attribute::getDereferenceableOrNullBytes() const {
assert(hasAttribute(Attribute::DereferenceableOrNull) &&
"Trying to get dereferenceable bytes from "
@@ -574,6 +581,9 @@ std::string Attribute::getAsString(bool InAttrGrp) const {
if (hasAttribute(Attribute::DereferenceableOrNull))
return AttrWithBytesToString("dereferenceable_or_null");
+ if (hasAttribute(Attribute::DeadOnReturn))
+ return AttrWithBytesToString("dead_on_return");
+
if (hasAttribute(Attribute::AllocSize)) {
unsigned ElemSize;
std::optional<unsigned> NumElems;
@@ -1162,6 +1172,10 @@ uint64_t AttributeSet::getDereferenceableBytes() const {
return SetNode ? SetNode->getDereferenceableBytes() : 0;
}
+uint64_t AttributeSet::getDeadOnReturnBytes() const {
+ return SetNode ? SetNode->getDeadOnReturnBytes() : 0;
+}
+
uint64_t AttributeSet::getDereferenceableOrNullBytes() const {
return SetNode ? SetNode->getDereferenceableOrNullBytes() : 0;
}
@@ -1366,6 +1380,12 @@ uint64_t AttributeSetNode::getDereferenceableBytes() const {
return 0;
}
+uint64_t AttributeSetNode::getDeadOnReturnBytes() const {
+ if (auto A = findEnumAttribute(Attribute::DeadOnReturn))
+ return A->getDeadOnReturnBytes();
+ return 0;
+}
+
uint64_t AttributeSetNode::getDereferenceableOrNullBytes() const {
if (auto A = findEnumAttribute(Attribute::DereferenceableOrNull))
return A->getDereferenceableOrNullBytes();
@@ -1983,6 +2003,10 @@ uint64_t AttributeList::getRetDereferenceableOrNullBytes() const {
return getRetAttrs().getDereferenceableOrNullBytes();
}
+uint64_t AttributeList::getDeadOnReturnBytes(unsigned Index) const {
+ return getParamAttrs(Index).getDeadOnReturnBytes();
+}
+
uint64_t
AttributeList::getParamDereferenceableOrNullBytes(unsigned Index) const {
return getParamAttrs(Index).getDereferenceableOrNullBytes();
@@ -2205,6 +2229,13 @@ AttrBuilder &AttrBuilder::addDereferenceableAttr(uint64_t Bytes) {
return addRawIntAttr(Attribute::Dereferenceable, Bytes);
}
+AttrBuilder &AttrBuilder::addDeadOnReturnAttr(uint64_t Bytes) {
+ if (Bytes == 0)
+ return *this;
+
+ return addRawIntAttr(Attribute::DeadOnReturn, Bytes);
+}
+
AttrBuilder &AttrBuilder::addDereferenceableOrNullAttr(uint64_t Bytes) {
if (Bytes == 0)
return *this;
diff --git a/llvm/lib/IR/Function.cpp b/llvm/lib/IR/Function.cpp
index 31a294447152e..d4e80762919c8 100644
--- a/llvm/lib/IR/Function.cpp
+++ b/llvm/lib/IR/Function.cpp
@@ -130,10 +130,9 @@ bool Argument::hasByValAttr() const {
return hasAttribute(Attribute::ByVal);
}
-bool Argument::hasDeadOnReturnAttr() const {
- if (!getType()->isPointerTy())
- return false;
- return hasAttribute(Attribute::DeadOnReturn);
+uint64_t Argument::getDeadOnReturnBytes() const {
+ assert(getType()->isPointerTy() && "Only pointers have dead_on_return bytes");
+ return getParent()->getDeadOnReturnBytes(getArgNo());
}
bool Argument::hasByRefAttr() const {
diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index 4ac1321860f66..16ab2d5f63dd5 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -1017,7 +1017,8 @@ struct DSEState {
// Treat byval, inalloca or dead on return arguments the same as Allocas,
// stores to them are dead at the end of the function.
for (Argument &AI : F.args())
- if (AI.hasPassPointeeByValueCopyAttr() || AI.hasDeadOnReturnAttr())
+ if (AI.hasPassPointeeByValueCopyAttr() ||
+ (AI.getType()->isPointerTy() && AI.getDeadOnReturnBytes() > 0))
InvisibleToCallerAfterRet.insert({&AI, true});
// Collect whether there is any irreducible control flow in the function.
diff --git a/llvm/test/Bitcode/attributes.ll b/llvm/test/Bitcode/attributes.ll
index 107a98aebeeb8..e908d95215c7d 100644
--- a/llvm/test/Bitcode/attributes.ll
+++ b/llvm/test/Bitcode/attributes.ll
@@ -577,8 +577,8 @@ define void @captures(ptr captures(address) %p) {
ret void
}
-; CHECK: define void @dead_on_return(ptr dead_on_return %p)
-define void @dead_on_return(ptr dead_on_return %p) {
+; CHECK: define void @dead_on_return(ptr dead_on_return(4) %p)
+define void @dead_on_return(ptr dead_on_return(4) %p) {
ret void
}
diff --git a/llvm/test/Bitcode/dead-on-return-upgrade.ll b/llvm/test/Bitcode/dead-on-return-upgrade.ll
new file mode 100644
index 0000000000000..d3fa2b8f91108
--- /dev/null
+++ b/llvm/test/Bitcode/dead-on-return-upgrade.ll
@@ -0,0 +1,7 @@
+; RUN: llvm-dis -o - %s.bc | FileCheck %s
+
+; CHECK: define void @test_dead_on_return_autoupgrade(ptr dead_on_return(18446744073709551615) %p) {
+
+define void @test_dead_on_return_autoupgrade(ptr dead_on_return %p) {
+ ret void
+}
diff --git a/llvm/test/Bitcode/dead-on-return-upgrade.ll.bc b/llvm/test/Bitcode/dead-on-return-upgrade.ll.bc
new file mode 100644
index 0000000000000..a2432d5a39b5b
Binary files /dev/null and b/llvm/test/Bitcode/dead-on-return-upgrade.ll.bc differ
diff --git a/llvm/test/Transforms/DeadStoreElimination/simple.ll b/llvm/test/Transforms/DeadStoreElimination/simple.ll
index 9d28395a4ccd0..3aa28a22f445b 100644
--- a/llvm/test/Transforms/DeadStoreElimination/simple.ll
+++ b/llvm/test/Transforms/DeadStoreElimination/simple.ll
@@ -856,7 +856,7 @@ bb:
ret void
}
-define void @test_dead_on_return(ptr dead_on_return %p) {
+define void @test_dead_on_return(ptr dead_on_return(4) %p) {
; CHECK-LABEL: @test_dead_on_return(
; CHECK-NEXT: ret void
;
@@ -864,7 +864,7 @@ define void @test_dead_on_return(ptr dead_on_return %p) {
ret void
}
-define void @test_dead_on_return_maythrow(ptr dead_on_return %p) {
+define void @test_dead_on_return_maythrow(ptr dead_on_return(4) %p) {
; CHECK-LABEL: @test_dead_on_return_maythrow(
; CHECK-NEXT: call void @maythrow()
; CHECK-NEXT: ret void
@@ -874,7 +874,7 @@ define void @test_dead_on_return_maythrow(ptr dead_on_return %p) {
ret void
}
-define ptr @test_dead_on_return_ptr_returned(ptr dead_on_return %p) {
+define ptr @test_dead_on_return_ptr_returned(ptr dead_on_return(4) %p) {
; CHECK-LABEL: @test_dead_on_return_ptr_returned(
; CHECK-NEXT: [[LOCAL_VAR:%.*]] = alloca ptr, align 8
; CHECK-NEXT: call void @opaque(ptr [[LOCAL_VAR]])
diff --git a/llvm/test/Verifier/dead-on-return.ll b/llvm/test/Verifier/dead-on-return.ll
index eb1d67fa319c2..da2c74e215457 100644
--- a/llvm/test/Verifier/dead-on-return.ll
+++ b/llvm/test/Verifier/dead-on-return.ll
@@ -1,7 +1,7 @@
; RUN: not llvm-as -disable-output %s 2>&1 | FileCheck %s
-; CHECK: Attribute 'dead_on_return' applied to incompatible type!
+; CHECK: Attribute 'dead_on_return(4)' applied to incompatible type!
; CHECK-NEXT: ptr @arg_not_pointer
-define void @arg_not_pointer(i32 dead_on_return %arg) {
+define void @arg_not_pointer(i32 dead_on_return(4) %arg) {
ret void
}
|
Created using spr 1.3.7
This patch makes the dead_on_return parameter attribute require a number
of bytes to be passed in to specify the number of bytes known to be dead
upon function return/unwind. This is aimed at enabling annotating the
this pointer in C++ destructors with dead_on_return in clang. We need
this to handle cases like the following:
```
struct X {
int n;
~X() {
this[n].n = 0;
}
};
void f() {
X xs[] = {42, -1};
}
```
Where we only certain that sizeof(X) bytes are dead upon return of ~X.
Otherwise DSE would be able to eliminate the store in ~X which would not
be correct.
This patch only does the wiring within IR. Future patches will make
clang emit correct sizing information and update DSE to only delete
stores to objects marked dead_on_return that are provably in bounds of
the number of bytes specified to be dead_on_return.
Reviewers:
Pull Request: llvm#171712
|
This will break a bunch of clang tests due to the textual IR changes. I'll get those updated once the rest of the patch is approved given that is just test churn. |
boomanaiden154
requested review from
alinas,
antoniofrighetto and
nikic
and removed request for
antoniofrighetto
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Created using spr 1.3.7
This patch makes the dead_on_return parameter attribute require a number
of bytes to be passed in to specify the number of bytes known to be dead
upon function return/unwind. This is aimed at enabling annotating the
this pointer in C++ destructors with dead_on_return in clang. We need
this to handle cases like the following:
```
struct X {
int n;
~X() {
this[n].n = 0;
}
};
void f() {
X xs[] = {42, -1};
}
```
Where we only certain that sizeof(X) bytes are dead upon return of ~X.
Otherwise DSE would be able to eliminate the store in ~X which would not
be correct.
This patch only does the wiring within IR. Future patches will make
clang emit correct sizing information and update DSE to only delete
stores to objects marked dead_on_return that are provably in bounds of
the number of bytes specified to be dead_on_return.
Reviewers:
Pull Request: llvm#171712
|
I can write up an RFC if people think there needs to be one for this. |
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
Can you share how the DSE changes will look like?
It looks like we currently fail to use dead_on_return inside isNotVisibleOnUnwind(), but that would also have to check the range with this approach. Possibly dead_on_unwind also needs the argument?
One thing I'm wondering is whether it makes sense to make the argument optional, to avoid the need to prove that the access is within specific bounds in the cases where it's not relevant (e.g. the original motivation for dead_on_return, which is byval-like arguments).
I have a rough prototype in boomanaiden154@c0972fb. I still need to double check some things (mostly that this correctly handles stores that straddle the boundaries and making sure I'm interpreting the API return values correctly in the case of non-constant offsets) and clean it up.
It would only need to have the range check if we make DSE understand that
That's definitely an option. I felt like the DSE changes did not look too expensive (just walking the use-def chain of the store pointer to calculate the offset from the original object), but I haven't collected any compile time numbers yet. This does pessimize the elimination of stores that have a variable offset from the base object, but I don't think that is the common case with either byval-like arguments or a destructor's |
IIRC DSE implicitly relies on the store also being dead on unwind if it's dead on return. Though I guess for that it's not strictly necessary to change dead_on_unwind, as long as we specify that dead on return only implies dead on unwind for the same range.
The variable offset case is the one I had in mind here. |
This patch makes the dead_on_return parameter attribute require a number
of bytes to be passed in to specify the number of bytes known to be dead
upon function return/unwind. This is aimed at enabling annotating the
this pointer in C++ destructors with dead_on_return in clang. We need
this to handle cases like the following:
Where we only certain that sizeof(X) bytes are dead upon return of ~X.
Otherwise DSE would be able to eliminate the store in ~X which would not
be correct.
This patch only does the wiring within IR. Future patches will make
clang emit correct sizing information and update DSE to only delete
stores to objects marked dead_on_return that are provably in bounds of
the number of bytes specified to be dead_on_return.