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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
