[IR] Make dead_on_return attribute optionally sized

[boomanaiden154]

This patch makes the dead_on_return parameter attribute optionally 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.

[llvmbot]

@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: Aiden Grossman (boomanaiden154)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/171712.diff

16 Files Affected:

• (modified) llvm/docs/LangRef.rst (+4-2)
• (modified) llvm/include/llvm/IR/Argument.h (+1-1)
• (modified) llvm/include/llvm/IR/Attributes.h (+12)
• (modified) llvm/include/llvm/IR/Attributes.td (+1-1)
• (modified) llvm/include/llvm/IR/Function.h (+6)
• (modified) llvm/lib/AsmParser/LLParser.cpp (+9-1)
• (modified) llvm/lib/Bitcode/Reader/BitcodeReader.cpp (+5)
• (modified) llvm/lib/IR/AttributeImpl.h (+1)
• (modified) llvm/lib/IR/Attributes.cpp (+31)
• (modified) llvm/lib/IR/Function.cpp (+3-4)
• (modified) llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp (+2-1)
• (modified) llvm/test/Bitcode/attributes.ll (+2-2)
• (added) llvm/test/Bitcode/dead-on-return-upgrade.ll (+7)
• (added) llvm/test/Bitcode/dead-on-return-upgrade.ll.bc ()
• (modified) llvm/test/Transforms/DeadStoreElimination/simple.ll (+3-3)
• (modified) llvm/test/Verifier/dead-on-return.ll (+2-2)

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
 }

[boomanaiden154]

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.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[boomanaiden154]

I can write up an RFC if people think there needs to be one for this.

[nikic]

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).

[boomanaiden154]

Can you share how the DSE changes will look like?

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 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?

It would only need to have the range check if we make DSE understand that dead_on_return implies dead_on_unwind if I am understanding things correctly. I think it should be reasonable to implement that in DSE without needing a sized dead_on_unwind. I haven't seen any motivating examples for a sized dead_on_unwind, but we also compile almost everything with -fno-exceptions, so I wouldn't be the one to see many.

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).

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 this pointer.

[nikic]

Can you share how the DSE changes will look like?

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 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?

It would only need to have the range check if we make DSE understand that dead_on_return implies dead_on_unwind if I am understanding things correctly. I think it should be reasonable to implement that in DSE without needing a sized dead_on_unwind. I haven't seen any motivating examples for a sized dead_on_unwind, but we also compile almost everything with -fno-exceptions, so I wouldn't be the one to see many.

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.

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).

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 this pointer.

The variable offset case is the one I had in mind here.

[boomanaiden154]

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.

I've updated the langref entry to mention that dead_on_return only implies dead_on_unwind for the specific range.

The variable offset case is the one I had in mind here.

Ack. I've updated the patch to preserve a unsized dead_on_return. I've implemented it as an integer attribute that uses the sentinel value of the uint64_t max to represent an unsized dead_on_return attribute instance. It looks like some of the other attributes wrap things in structs, but that doesn't seem necessary here given std::optional appropriately captures what we need and is also seemingly a canonical design pattern.

[antoniofrighetto]

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.

I've updated the langref entry to mention that dead_on_return only implies dead_on_unwind for the specific range.

Missing change?

[antoniofrighetto]

I wonder if it'd be possible to make it a bit simpler to figure out we're only interested in non-sized dead_on_return here (it may not be immediately clear). Maybe worth a comment (even though may carry out changes here later)?

[boomanaiden154]

Good point. I've updated the patch to change the getter to getDeadOnReturnInfo that returns a struct wrapping the optional to appropriately name functions. So now we can just query coversAllRechableMemory() which should be self-documenting. Name bike-shedding is welcome since I'm not super confident that is a great name.