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

[David Tellenbach via Phabricator via cfe-commits]

tellenbach added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74094