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

nickdesaulniers updated this revision to Diff 557122. nickdesaulniers added a comment. - git clang-format HEAD~ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74094/new/ https://reviews.llvm.org/D74094 Files: clang/docs/ReleaseNotes.rst

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

nickdesaulniers updated this revision to Diff 557121. nickdesaulniers added a comment. - re-add sentence I accidentally dropped from the release note Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74094/new/ https://reviews.llvm.org/D74094 Files:

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

nickdesaulniers updated this revision to Diff 557119. nickdesaulniers added a comment. - undo ReturnsReference checks, add -Xclang -sloppy-temporary-lifetimes, add test for that, add release note Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

nickdesaulniers added a comment. In D74094#4648319 , @rjmccall wrote: >> That's better than the status quo (no lifetime markers at all) but still >> suboptimal, and I don't think it will solve the particular case I care about >> (a particular Linux

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

rjmccall added a comment. > That's better than the status quo (no lifetime markers at all) but still > suboptimal, and I don't think it will solve the particular case I care about > (a particular Linux kernel driver written in C which is passing many > aggregates by value). Ah, all in the

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

nickdesaulniers added a comment. (Sorry for the wall of text) In 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): Woah! Deus

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

xbolva00 added a comment. Yes, cc1 flag would be useful. 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

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

rjmccall added a comment. In D74094#4646975 , @xbolva00 wrote: >>> 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

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

xbolva00 added a comment. >> 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

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

rjmccall added a comment. In 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

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

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().cast(); } does not return a Tensor but an expression encoding the

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

nickdesaulniers added a comment. In D74094#4646152 , @nickdesaulniers wrote: > In D74094#4645998 , @dexonsmith > wrote: > >> In D74094#4645562 , >> @nickdesaulniers

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

nickdesaulniers added a comment. In D74094#4645998 , @dexonsmith wrote: > In D74094#4645562 , @nickdesaulniers > wrote: > >> I don't yet fully comprehend yet what's going wrong, and probably need to >>

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

dexonsmith added a comment. In D74094#4645562 , @nickdesaulniers wrote: > I don't yet fully comprehend yet what's going wrong, and probably need to > familiarize myself with the language rules around `auto`'s type deduction. For reduction purposes, it

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

nickdesaulniers added a comment. In D74094#4643653 , @nickdesaulniers wrote: > With this change re-applied (`b7f4915644844fb9f32e8763922a070f5fe4fd29` > reverted), a.out runs without issue. So yes, I need something other than a > godbolt link which

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

rjmccall added a comment. > 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. I can see how my response was somewhat aggressive, and I regret that and apologize. I imagine you're probably

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

nickdesaulniers added a comment. In D74094#4643577 , @rjmccall wrote: > In D74094#4643558 , @nickdesaulniers > wrote: > >> In D74094#4643495 , @rjmccall wrote: >> >>> I

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

rjmccall added a comment. In D74094#4643558 , @nickdesaulniers wrote: > In D74094#4643495 , @rjmccall wrote: > >> I can't easily tell you because the "standalone example" involves a million >> templates that I

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

nickdesaulniers added a comment. In 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

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

rjmccall added a comment. 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. What I can tell you is that there is an enormous

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

nickdesaulniers planned changes to this revision. nickdesaulniers added a comment. In D74094#4643439 , @rjmccall wrote: > Under [expr.call]p6 (https://eel.is/c++draft/expr.call#6), it is > implementation-defined whether the lifetime of a parameter ends

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

rjmccall added a comment. Under [expr.call]p6 (https://eel.is/c++draft/expr.call#6), it is implementation-defined whether the lifetime of a parameter ends at the end of the containing full expression or at the end of the function. If the implementation chooses the latter, a reference to the

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

nickdesaulniers updated this revision to Diff 556461. nickdesaulniers added a comment. This revision is now accepted and ready to land. - skip optimization when returning a reference; regardless of type - fix test run line - use --check-prefixes= - reflow RUN lines Repository: rG LLVM Github

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

nickdesaulniers added inline comments. Comment at: clang/test/CodeGen/lifetime-call-temp.c:54 + + // CXX-NOT @llvm.lifetime + // CXX: ret void this check line looks broken; missing `:` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

nickdesaulniers added a comment. In D74094#4634966 , @rjmccall wrote: > Parameter objects are not temporaries and have their own lifetime rules, so > there's nothing wrong with this idea in principle. This seems to just be a > bug, probably that we're

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

rjmccall added a comment. Parameter objects are not temporaries and have their own lifetime rules, so there's nothing wrong with this idea in principle. This seems to just be a bug, probably that we're doing a type check on `E->getType()` without considering whether E might be a gl-value. We

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

vitalybuka added a comment. In D74094#4633785 , @alexfh wrote: > This commit caused invalid AddressSanitizer: stack-use-after-scope errors in > our internal setup. Trying to create a standalone repro, but so far we think > that the patch is incorrect.

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

alexfh added a comment. In D74094#4633785 , @alexfh wrote: > This commit caused invalid AddressSanitizer: stack-use-after-scope errors in > our internal setup. Trying to create a standalone repro, but so far we think > that the patch is incorrect.

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

alexfh added subscribers: vitalybuka, alexfh. alexfh added a comment. This commit caused invalid AddressSanitizer: stack-use-after-scope errors in our internal setup. Trying to create a standalone repro, but so far we think that the patch is incorrect. @vitalybuka says "The patch seems wrong,

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

nickdesaulniers added a comment. Thanks @lei for the help testing. Re-applied. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74094/new/ https://reviews.llvm.org/D74094 ___ cfe-commits mailing list

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGe698695fbbf6: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument… (authored by erik.pilkington, committed by nickdesaulniers). Repository: rG LLVM Github Monorepo CHANGES

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

dexonsmith added a comment. Nice! 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

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

lei added a comment. @nickdesaulniers I have verified this patch on top of `40ee8abee77a2e8fb0089d4c7f5723b71f27d416` passes our multistage bot http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage Thank-you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

lei added a comment. Sorry I missed this. Will kick off now and let you know the results soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74094/new/ https://reviews.llvm.org/D74094 ___ cfe-commits

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

nickdesaulniers added a comment. In D74094#4566543 , @nickdesaulniers wrote: > Hi @lei can you please help us retest this version of this patch that > previously broke your multistage build? >

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

nickdesaulniers updated this revision to Diff 547861. nickdesaulniers added a comment. - remove another auto, sink ArgSlotAlloca to reduce scope Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74094/new/ https://reviews.llvm.org/D74094 Files:

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

nickdesaulniers added a comment. Hi @lei can you please help us retest this version of this patch that previously broke your multistage build? https://reviews.llvm.org/rGfafc6e4fdf3673dcf557d6c8ae0c0a4bb3184402#890444 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

nickdesaulniers updated this revision to Diff 547853. nickdesaulniers added a comment. - remove instance of auto Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74094/new/ https://reviews.llvm.org/D74094 Files: clang/lib/CodeGen/CGCall.cpp

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

nickdesaulniers updated this revision to Diff 547846. nickdesaulniers added a comment. This revision is now accepted and ready to land. - fix remaining test failures Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74094/new/

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

nickdesaulniers updated this revision to Diff 547271. nickdesaulniers retitled this revision from "[IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas" to "Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas". nickdesaulniers edited the