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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

> 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 <https://reviews.llvm.org/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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116998/new/

https://reviews.llvm.org/D116998