nickdesaulniers added a comment. In D74094#4643577 <https://reviews.llvm.org/D74094#4643577>, @rjmccall wrote:
> 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. I've cloned: 1. abseil @ f3eae68bd1d17df708f2b59a43ad7e837a616e6a 2. eigen @ 8f858a4ea8106b52f3cf475ffc5e0e9c6a91baa2 3. googletest @ eab0e7e289db13eabfc246809b0284dac02a369d I had to build googletest. Then I built a.out via: $ clang++ foo.cpp -I eigen/ -I googletest/googletest/include/ \ -I googletest/googlemock/include/ -I abseil-cpp googletest/build/lib/libgtest.a \ -std=c++17 -O3 -fsanitize=address -march=haswell With this change re-applied (`b7f4915644844fb9f32e8763922a070f5fe4fd29` reverted), a.out runs without issue. So yes, I need something other than a godbolt link which uses clang-trunk which isn't stable after a revert to understand what they are talking about. >>> 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. Fuck me for trying to help, right? If x-values are part of the "basics" of parameter passing, I'm afraid to ask about the more complicated cases. > 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);`. Thanks, that's //all// I needed. With `b7f4915644844fb9f32e8763922a070f5fe4fd29` reverted, neither of those cases look fishy to me: https://gist.github.com/nickdesaulniers/1c7b962222639e54c3339b63b572b166 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
