[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#4643558 <https://reviews.llvm.org/D74094#4643558>, @nickdesaulniers wrote:

> In D74094#4643495 <https://reviews.llvm.org/D74094#4643495>, @rjmccall wrote:
>
>> I can't easily tell you because the "standalone example" involves a million templates that I don't know anything about, and nobody seems to have done the analysis to identify the specific source of the miscompile.
>
> Sure; @alexfh @vitalybuka can you both please help produce a further reduced test case from https://reviews.llvm.org/D74094#4633799 ? Otherwise I'm beginning to think this patch may have been reverted prematurely; before an understood + reduced test case was reduced.

Well, it's good to be conservative about adding new optimizations if they're causing apparent miscompiles.  Even if the optimization is just exposing existing UB in some new one, that's something we should understand as a project.  I have no problem with the eager revert.

It would be great if Alex and Vitaly could help track down the exact problem.  However, if they can't, I think it's ultimately your responsibility as the person hoping to land the optimization to understand why the miscompile is not in fact your fault.

>> What I can tell you is that there is an enormous semantic difference between passing a pr-value argument to a parameter that's passed by reference and to a parameter that's passed by value.  In the former case, the reference is bound to a temporary with full-expression lifetime, and it is totally reasonable to return its address.  In the latter case, the parameter object will often have a lifetime bound by the current function, and you cannot return its address without it being UB.
>
> I'm trying to come up with an example of "the former."
>
>   struct foo {
>     int x;
>   };
>   
>   // Returns a reference to the minimum foo.
>   foo &min(foo &a, foo &b);
>   
>   void consume (foo&);
>   
>   void bar () {
>     consume(min(foo{0}, foo{1}));
>   }
>
> doesn't compile:
>
>   foo.cpp:11:11: error: no matching function for call to 'min'
>      11 |   consume(min(foo{0}, foo{1}));
>         |           ^~~
>   foo.cpp:6:6: note: candidate function not viable: expects an lvalue for 1st argument
>       6 | foo &min(foo &a, foo &b);
>         |      ^   ~~~~~~
>
> Have I misinterpreted what you meant by "passing a pr-value argument to a parameter that's passed by reference?"

Okay, to be frank, this is a bit of a red flag for me.  Optimizations like this are ultimately rooted in language rules, and I'm not sure you can effectively do this work if you don't understand the basics of C++ parameter passing.  In C++, you cannot bind a non-const l-value reference to a temporary, but you can bind either a const l-value reference or an x-value reference to one.  So you could declare `min` as either `const foo &min(const foo &a, const foo &b);` or `foo &&min(foo &&a, foo &&b);`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74094