[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D74094#4646297 <https://reviews.llvm.org/D74094#4646297>, @tellenbach wrote:

> No real comment on the issue itself but on the example as a former Eigen maintainer (sorry for the noise if that's all obvious for you):
>
>   auto round (Tensor m) {
>       return (m + 0.5f).cast<int>().cast<float>();
>   }
>
> does not return a Tensor but an expression encoding the computation which is evaluated during the assignment `const Tensor t3 = round(a.log().exp());` using an overloaded `operator=`. With "evaluation" I mean evaluation the in the sense of performing the intended mathematical computation.
>
> Many things can go wrong when using `auto` in such cases, of which two seem to be relevant here:
>
> 1. Eigen can (this is an implementation detail(!)) decide to evaluate subexpressions into temporaries. The returned expression would then reference the result of such an evaluation beyond its lifetime.
> 2. If 1. does not happen, the unevaluated expression would reference its arguments. The immediate `0.5f` is copied since that's cheap, but the tensor would be referenced. Your example function `round` takes its parameter by-value and the returned expression would reference it. I'm unsure if the lifetime would be extended in this case (again, the exact details of C++ lifetime rules are not my area of expertise). I think if the lifetime would be extended, ASAN complaining after applying this patch is wrong, if the lifetime is not extended ASAN should complain and the example is wrong.
>
> Btw, these issues are so common that Eigen documents the recommendation to avoid using `auto` with Eigen types all together: https://eigen.tuxfamily.org/dox/TopicPitfalls.html#title3

Okay, thanks, I can see how that works, and I definitely would never had figured that out from just looking at this code.  The formal lifetime of a parameter of type `Tensor` is definitely in the land of implementation-defined behavior, so this code seems to be at best non-portable.

Nick, maybe we can take a new look at this patch from that perspective.  You've been trying to use very tight lifetime bounds for these objects that only cover the duration of call, which essentially means you're defining the lifetime of parameter objects to just be the call rather than the full-expression (at least, when the parameter type doesn't have a destructor).  In the abstract, that's probably the right rule for Clang to adopt, because I think it matches programmer intuition (it's always wrong to return the address of a local, and that includes by-value parameters), and it means we'd have a consistent rule for all types and targets.  It's also a fairly aggressive rule that's likely to uncover a fair amount of code like this that's assuming longer lifetimes.  I think it's reasonable to say that these examples are all program bugs that should be fixed, but it might be better to pursue that as a *refinement* on top of an initially more conservative rule.  I suspect that that conservative rule will also give us a large fraction of the optimization benefit that we can expect from this change, because at least parameter objects from calls in different full-expressions will be able to share memory.  So we can start by giving these objects full-expression lifetime, chase down any program bugs that that uncovers (which will all *unquestionably* be program bugs under the standard), and then gradually work on landing the more aggressive rule (perhaps even for non-trivial types).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74094