<div dir="ltr"><div>I think this is correct as written. See below for some suggestions on how to clean this up. I think it's important to move the pushing of the lifetime ending into pushTemporaryCleanup because the storage duration switch should mirror pushing the destructor cleanup.</div><div><br></div><div>----</div><div><br></div><div>+CodeGenFunction::MoveDefferedCleanups(size_t OldLifetimeExtendedSize) {</div><div><br></div><div>"Deferred", as you have it spelled elsewhere.</div><div><br></div><div><div>+ uint64_t size =</div><div>+ CGM.getDataLayout().getTypeStoreSize(ConvertTypeForMem(E->getType()));</div><div>+ bool useLifetimeMarkers =</div><div>+ (M->getStorageDuration() == SD_FullExpression ||</div><div>+ M->getStorageDuration() == SD_Automatic) &&</div><div>+ EmitLifetimeStart(size, Object);</div></div><div><br></div><div>LLVM's coding standards use leading uppercase letters for local variables, for better or worse. This happens in a number of other places, please try to fix the other instances.</div><div><br></div><div><div>+/// Emit a lifetime.begin marker if some criteria are satisfied.</div><div>+/// \return true if a marker was emitted, false otherwise</div><div>+bool CodeGenFunction::EmitLifetimeStart(uint64_t Size, llvm::Value *Addr) {</div><div>+ if (!shouldUseLifetimeMarkers(*this, Size))</div><div>+ return false;</div><div>+</div><div>+ llvm::Value *SizeV = llvm::ConstantInt::get(Int64Ty, Size);</div><div>+ llvm::Value *castAddr = Builder.CreateBitCast(Addr, Int8PtrTy);</div><div>+ Builder.CreateCall2(CGM.getLLVMLifetimeStartFn(), SizeV, castAddr)</div><div>+ ->setDoesNotThrow();</div><div>+ return true;</div><div>+}</div></div><div><br></div><div>I think this can be simplified by returning SizeV or nullptr, and assigning the result to SizeForLifeTimeMarkers unconditionally.</div><div><br></div><div><div> llvm::Value *Object = createReferenceTemporary(*this, M, E);</div><div>+</div><div>+ uint64_t size =</div><div>+ CGM.getDataLayout().getTypeStoreSize(ConvertTypeForMem(E->getType()));</div><div>+ bool useLifetimeMarkers =</div><div>+ (M->getStorageDuration() == SD_FullExpression ||</div><div>+ M->getStorageDuration() == SD_Automatic) &&</div><div>+ EmitLifetimeStart(size, Object);</div><div>+</div><div><br></div><div>I think you should fold this into createReferenceTemporary(), and add an output parameter like SizeForLifeTimeMarkers that gets passed into pushTemporaryCleanup.</div><div><br></div><div> if (auto *Var = dyn_cast<llvm::GlobalVariable>(Object)) {</div><div> // If the temporary is a global and has a constant initializer, we may</div><div> // have already initialized it.</div><div>@@ -363,6 +371,24 @@ LValue CodeGenFunction::EmitMaterializeTemporaryExpr(</div><div> } else {</div><div> EmitAnyExprToMem(E, Object, Qualifiers(), /*IsInit*/true);</div><div> }</div><div>+</div><div>+ if (useLifetimeMarkers) {</div><div>+ llvm::Value *sizeV = llvm::ConstantInt::get(Int64Ty, size);</div><div>+ switch (M->getStorageDuration()) {</div><div>+ case SD_FullExpression:</div><div>+ pushFullExprCleanup<CallLifetimeEnd>(NormalAndEHCleanup, Object, sizeV);</div><div>+ break;</div><div>+ case SD_Automatic:</div><div>+ EHStack.pushCleanup<CallLifetimeEnd>(static_cast<CleanupKind>(EHCleanup),</div><div>+ Object, sizeV);</div><div>+ pushCleanupAfterFullExpr<CallLifetimeEnd>(NormalAndEHCleanup, Object,</div><div>+ sizeV);</div><div>+ break;</div><div>+ default:</div><div>+ llvm_unreachable("unexpected storage duration for Lifetime markers");</div><div>+ }</div><div>+ }<br></div><div><br></div><div>IMO this should be in pushTemporaryCleanup, right before the early return for types that don't have destructors. This way we unify the behavior of ARC reference temporaries with normal temporaries.</div><div><br></div><div>+</div><div> pushTemporaryCleanup(*this, M, E, Object);</div></div><div><br></div><div><div>+ /// A cleanup to call @llvm.lifetime.end.</div><div>+ class CallLifetimeEnd : public EHScopeStack::Cleanup {</div><div>+ llvm::Value *Addr;</div><div>+ llvm::Value *Size;</div><div>+ public:</div><div>+ CallLifetimeEnd(llvm::Value *addr, llvm::Value *size)</div><div>+ : Addr(addr), Size(size) {}</div><div>+</div><div>+ void Emit(CodeGenFunction &CGF, Flags flags) override {</div><div>+ CGF.EmitLifetimeEnd(Size, Addr);</div><div>+ }</div><div>+ };</div></div><div><br></div><div>Subclasses of EHScopeStack::Cleanup are generally declared in anonymous namespaces, according to this comment:</div><div><div> /// Cleanup implementations should generally be declared in an</div><div> /// anonymous namespace.</div></div><div><br></div><div>We don't have any instances currently violating this rule, so I think it'd be best to surface CodeGenFunction::pushLifeTimeEnd similar to the family of push*Destroy methods. I think I was the one who suggested hoisting this into a header, but I wasn't thinking carefully about it. Woops. :(</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 23, 2014 at 9:06 AM, Arnaud A. de Grandmaison <span dir="ltr"><<a href="mailto:arnaud.degrandmaison@arm.com" target="_blank">arnaud.degrandmaison@arm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-GB" link="blue" vlink="purple"><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Ping<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm"><p class="MsoNormal" style="margin-left:36.0pt"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> <a href="mailto:cfe-commits-bounces@cs.uiuc.edu" target="_blank">cfe-commits-bounces@cs.uiuc.edu</a> [mailto:<a href="mailto:cfe-commits-bounces@cs.uiuc.edu" target="_blank">cfe-commits-bounces@cs.uiuc.edu</a>] <b>On Behalf Of </b>Arnaud A. de Grandmaison<br><b>Sent:</b> 18 September 2014 16:35<br><b>To:</b> llvm cfe<br><b>Subject:</b> RE: [PATCH] Emit lifetime start/end for unnamed objects --- take 3<u></u><u></u></span></p></div></div><div><div class="h5"><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p><p class="MsoNormal" style="margin-left:36.0pt"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Gentle ping: could someone familiar with cleanup scopes and lifetime extended temporaries have a look at the patch (attached again to this mail for convenience)?<u></u><u></u></span></p><p class="MsoNormal" style="margin-left:36.0pt"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal" style="margin-left:36.0pt"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Cheers,<u></u><u></u></span></p><p class="MsoNormal" style="margin-left:36.0pt"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Arnaud<u></u><u></u></span></p><p class="MsoNormal" style="margin-left:36.0pt"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal" style="margin-left:72.0pt"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> David Blaikie [<a href="mailto:dblaikie@gmail.com" target="_blank">mailto:dblaikie@gmail.com</a>] <br><b>Sent:</b> 16 September 2014 20:50<br><b>To:</b> Arnaud De Grandmaison<br><b>Cc:</b> llvm cfe<br><b>Subject:</b> Re: [PATCH] Emit lifetime start/end for unnamed objects --- take 3<u></u><u></u></span></p><p class="MsoNormal" style="margin-left:72.0pt"><u></u> <u></u></p><div><p class="MsoNormal" style="margin-left:72.0pt"><u></u> <u></u></p><div><p class="MsoNormal" style="margin-left:72.0pt"><u></u> <u></u></p><div><p class="MsoNormal" style="margin-left:72.0pt">On Tue, Sep 16, 2014 at 1:29 AM, Arnaud A. de Grandmaison <<a href="mailto:arnaud.degrandmaison@arm.com" target="_blank">arnaud.degrandmaison@arm.com</a>> wrote:<u></u><u></u></p><div><div><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">Hi David,</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">In principal, giving more lifetime info can only improve stack slot sharing and reduce runtime stack usage, which is critical for us in the embedded world.</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">I did not test that on a wide code base yet, but we had a customer reporting an issue where llvm/clang was producing code with a stack usage significantly worse than gcc. To make things worse, in their case their code was heavily recursive, to the point where using clang was simply not an option: they are forced to use gcc </span><span style="font-size:11.0pt;font-family:Wingdings;color:#1f497d">L</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">This patch only adds lifetime markers to big enough (>32 bytes) objects, consistent with what is done for named temporaries. I do not know how this 32 bytes threshold has been choosen, but there is for sure a compile time / stack size gain trade-off to be made. My experiments have shown that for our customer case, the threshold should be lower: 16-bytes. But changing this threshold would require a separate thread on this list, as well as much more measurements.</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">The improvements I have been able to get, by visual inspection of the generated assembly code, for a single call of the hot functions were:</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> | GCC | Clang | LT-32 | LT-16 |</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">===+=====+=======+=======+=======+</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">F1 | 432 | 608 | 608 | 400 |</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">F2 | 432 | 640 | 640 | 432 |</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">F3 | 384 | 368 | 368 | 192 |</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">F4 | 320 | 400 | 400 | 224 |</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">Stack size is expressed in bytes.</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">GCC version 4.8</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">LT-32 is clang with this patch (default 32 bytes threshold for all temporaries).</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">LT-16 is clang with this patch and a 16 bytes threshold for all temporaries.</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">I believe bootstrapping clang could be a good testcase and will be needed when we will address the real problem in a separate discussion: what threshold should we use ?</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">Very strangely to me coming from the embedded world, I have not found how to measure a program stack usage on linux, so if you have any idea, I am glad to hear about it.</span><u></u><u></u></p></div></div><div><p class="MsoNormal" style="margin-left:72.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:72.0pt">I'm not sure what the nicest way to do it for the running program or examining a binary, but I would /imagine/ that LLVM might have a counter/statistic for stack usage. I believe LLVM has some way to record statistics about optimizations, etc, for debugging the compiler. So if it doesn't have a "stack size" stat counter, it could hopefully be added.<br><br>But you're right, for now - adding more should be generally better. I'm not sure if there's a concern that adding too many (given that there's a threshold, I assume someone tried it without a threshold and found that it created too much metadata) intrinsics like this - so there might be some need to show cost/benefit... (maybe looking at the commit archive to find the original commits, those that added the threshold, etc)<br><br>(this is way out of my depth/area of interest - but just replying with suggestions/ideas to both make the conversation more visible, give you some ideas that might pre-empt ideas from other more knowledgeable reviewers, etc)<br><br>- David<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:72.0pt"> <u></u><u></u></p></div><blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0cm;margin-bottom:5.0pt"><div><div><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">Cheers,</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d">Arnaud</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> David Blaikie [mailto:<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>] <br><b>Sent:</b> 16 September 2014 01:25<br><b>To:</b> Arnaud De Grandmaison<br><b>Cc:</b> llvm cfe<br><b>Subject:</b> Re: [PATCH] Emit lifetime start/end for unnamed objects --- take 3</span><u></u><u></u></p><div><div><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p><div><p class="MsoNormal" style="margin-left:108.0pt">I'm hardly an expert on this stuff - but just curious: what sort of testing did you put this through? Bootstrap Clang? Were you able to gather any stats on reduced stack usage with this improvement to lifetime markers?<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p><div><p class="MsoNormal" style="margin-left:108.0pt">On Mon, Sep 15, 2014 at 4:05 PM, Arnaud A. de Grandmaison <<a href="mailto:arnaud.degrandmaison@arm.com" target="_blank">arnaud.degrandmaison@arm.com</a>> wrote:<u></u><u></u></p><div><div><p class="MsoNormal" style="margin-left:108.0pt">Hi All,<u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt">Please find attached a patch which teaches clang to emit lifetime.start / lifetime.end markers for unnamed temporary objects.<u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt">This patch can greatly reduce the stack usage of some C++ code, where it is so easy to have short lived unnamed temporaries.<u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt">As noted in the subject, this is my third attempt: my previous attempts failed to handle correctly the lifetime extended temporaries, and I have had a hard time to understand the CleanupScope. It all boiled down to the fact that the body of a function is not considered a full CleanupScope (for debug information reasons), so in the case of lifetime extended objects at the top level of the function body, with a trivial destructor + lifetime.end marker, the lifetime markers were simply not considered, firing an assert in ~CodeGenFunction. All cases are now covered by testcases.<u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt">I would appreciate if someone knowledgeable with the lifetime extended temporaries & cleanup scopes could give a look to this patch.<u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt">Cheers,<u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt">--<u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt">Arnaud A. de Grandmaison<u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p></div></div><p class="MsoNormal" style="margin-bottom:12.0pt;margin-left:108.0pt"><br>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><u></u><u></u></p></div><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p></div></div></div></div></div></blockquote></div><p class="MsoNormal" style="margin-left:72.0pt"><u></u> <u></u></p></div></div></div></div></div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div>