[LangRef] Don't allow read from sret memory after unwind

[nikic]

Summary

Following up on the discussion in https://groups.google.com/g/llvm-dev/c/i0Z1FC51KVI, this updates sret semantics to specify that the sret memory cannot be read after unwinding. This enables optimizations like the following:

declare void @may_unwind()

define void @src(i32* noalias sret(i32) %out) {
    store i32 0, i32* %out
    call void @may_unwind()
    store i32 1, i32* %out
    ret void
}

define void @tgt(i32* noalias sret(i32) %out) {
    call void @may_unwind()
    store i32 1, i32* %out
    ret void
}

Without the guarantee, the memory state of %out could be observed if @may_unwind() unwinds, and the first store would not be dead.

Rather than making accesses after unwind UB, this instead specifies that the memory is filled with poison. This gives us the necessary optimization guarantees without preventing accesses entirely (e.g. "lifetime.end" on unwind must remain legal, and is currently modeled as an access.)

[jdoerfert]

FWIW, I'm fine with this.

[reames]

This looks reasonable, but I don't really have the context on sret to understand any implicit assumptions made in current usage. I'd encourage you to find someone with current context to approve. If you can't, I can take the time to build it, but that's somewhat expensive time wise, so I'd really prefer not to it.

[nikic]

In D116998#3234540, @reames wrote:

This looks reasonable, but I don't really have the context on sret to understand any implicit assumptions made in current usage. I'd encourage you to find someone with current context to approve. If you can't, I can take the time to build it, but that's somewhat expensive time wise, so I'd really prefer not to it.

Not sure who would be familiar, maybe @rnk.

The intuition here is that sret is basically the function return value, just passed indirectly, and you can't read a function return value on unwind.

[nikic]

Ping :)

[nikic]

Hum, I just found this wonderful bit of code in ArgPromotion: https://github.com/llvm/llvm-project/blob/e9768a2a44a1501b82e3bbf9862b4ba2cc4b9cc3/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp#L932-L943 It replaces sret arguments with noalias arguments. So ArgPromotion could lose optimization information if the "poison on unwind" behavior is not also encoded by a separate attribute.

I find this particularly odd because I always assumed that frontends are encouraged to annotate struct return value arguments as sret. But now it looks like frontends actually shouldn't do that unless they must match C ABI?

[rnk]

I find this particularly odd because I always assumed that frontends are encouraged to annotate struct return value arguments as sret. But now it looks like frontends actually shouldn't do that unless they must match C ABI?

I've always thought of sret as an ABI attribute, not a semantic attribute, but I don't think there's wide agreement on that.

I think the value of that ArgPromotion transform is somewhat questionable. In cases that don't involve NRVO, the sret pointer is used as part of the return statement, so it has a long live range anyway, and this transform has no value.

There are some cases with NRVO where the return value can be initialized very early and then never used until the return, something like:

SRet foo() {
  SRet rv{};
  // use all the registers
  return rv;
}

In any case, it feels like this transform is working around a limitation or bug in codegen. I have previously observed bad codegen, where LLVM spills the sret pointer into two stack slots for no reason. It might be worth looking into that.

---

After going through blame, that transform was introduced in D10353 (2015). The main reason was to avoid verifier errors caused by argument promotion firing on this. The verifier still requires that sret appear on the first or second parameter, and promoting the first parameter violated that rule:
https://github.com/llvm/llvm-project/blob/f4744e9ae08f70ce416bedeedc1af64f29a20970/llvm/lib/IR/Verifier.cpp#L1945

Honestly, I think that's the real reason this transform exists. I don't think we did any performance measurements in 2015 to motivate this transform. I think if you remove the verifier rule and audit backends to ensure they handle sret in any position, we could remove this transform.

[nikic]

As changing sret semantics seems to be somewhat problematic, I've created a new patch that introduces a separate attribute instead: https://reviews.llvm.org/D157499 That also has the benefit of being inferable, at least in principle, because it's not mixed in with ABI considerations.

llvm/docs/LangRef.rst

``inalloca(<ty>)``
     See :doc:`InAlloca` for more information on how to use this
     attribute.
 
 ``sret(<ty>)``
     This indicates that the pointer parameter specifies the address of a
     structure that is the return value of the function in the source
     program. This pointer must be guaranteed by the caller to be valid:
     loads and stores to the structure may be assumed by the callee not
-    to trap and to be properly aligned. This is not a valid attribute
-    for return values.
+    to trap and to be properly aligned.
+
+    If the call unwinds, then the underlying object associated with the
+    argument is overwritten with a :ref:`poison value <poisonvalue>`.
+    As such, subsequent reads cannot depend on the object containing any
+    particular value.
 
     The sret type argument specifies the in memory type, which must be
-    the same as the pointee type of the argument.
+    the same as the pointee type of the argument. This is not a valid
+    attribute for return values.
 
 .. _attr_elementtype:
 
 ``elementtype(<ty>)``
 
     The ``elementtype`` argument attribute can be used to specify a pointer
     element type in a way that is compatible with `opaque pointers
     <OpaquePointers.html>`.