<div dir="ltr">Sorry for the wait, this fell off my radar and I thought it landed. Looks good, please commit!</div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Sep 27, 2014 at 4:00 PM, 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">Hi Reid,<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><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Here is an updated patch with all your comments taken into account.<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><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">There is however one difference with the original patch: lifetime markers are also inserted in the ObjC case (in EmitMaterializeTemporaryExpr). I do not know if this could be a problem though. Do you have any opinion there ?<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><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Thanks again for your review.<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><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Cheers,<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Arnaud<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><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""> Reid Kleckner [mailto:<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>] <br><b>Sent:</b> 27 September 2014 00:21<br><b>To:</b> Arnaud De Grandmaison<br><b>Cc:</b> llvm cfe; Richard Smith</span></p><div><div class="h5"><br><b>Subject:</b> Re: [PATCH] Emit lifetime start/end for unnamed objects --- take 3<u></u><u></u></div></div><p></p><div><div class="h5"><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p><div><div><p class="MsoNormal" style="margin-left:36.0pt">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.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">----<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+CodeGenFunction::MoveDefferedCleanups(size_t OldLifetimeExtendedSize) {<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">"Deferred", as you have it spelled elsewhere.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><div><p class="MsoNormal" style="margin-left:36.0pt">+  uint64_t size =<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      CGM.getDataLayout().getTypeStoreSize(ConvertTypeForMem(E->getType()));<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  bool useLifetimeMarkers =<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      (M->getStorageDuration() == SD_FullExpression ||<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+       M->getStorageDuration() == SD_Automatic) &&<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      EmitLifetimeStart(size, Object);<u></u><u></u></p></div></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">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.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><div><p class="MsoNormal" style="margin-left:36.0pt">+/// Emit a lifetime.begin marker if some criteria are satisfied.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+/// \return true if a marker was emitted, false otherwise<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+bool CodeGenFunction::EmitLifetimeStart(uint64_t Size, llvm::Value *Addr) {<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  if (!shouldUseLifetimeMarkers(*this, Size))<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    return false;<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  llvm::Value *SizeV = llvm::ConstantInt::get(Int64Ty, Size);<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  llvm::Value *castAddr = Builder.CreateBitCast(Addr, Int8PtrTy);<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  Builder.CreateCall2(CGM.getLLVMLifetimeStartFn(), SizeV, castAddr)<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      ->setDoesNotThrow();<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  return true;<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+}<u></u><u></u></p></div></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">I think this can be simplified by returning SizeV or nullptr, and assigning the result to SizeForLifeTimeMarkers unconditionally.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><div><p class="MsoNormal" style="margin-left:36.0pt">   llvm::Value *Object = createReferenceTemporary(*this, M, E);<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  uint64_t size =<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      CGM.getDataLayout().getTypeStoreSize(ConvertTypeForMem(E->getType()));<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  bool useLifetimeMarkers =<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      (M->getStorageDuration() == SD_FullExpression ||<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+       M->getStorageDuration() == SD_Automatic) &&<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      EmitLifetimeStart(size, Object);<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">I think you should fold this into createReferenceTemporary(), and add an output parameter like SizeForLifeTimeMarkers that gets passed into pushTemporaryCleanup.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">   if (auto *Var = dyn_cast<llvm::GlobalVariable>(Object)) {<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">     // If the temporary is a global and has a constant initializer, we may<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">     // have already initialized it.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">@@ -363,6 +371,24 @@ LValue CodeGenFunction::EmitMaterializeTemporaryExpr(<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">   } else {<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">     EmitAnyExprToMem(E, Object, Qualifiers(), /*IsInit*/true);<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">   }<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  if (useLifetimeMarkers) {<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    llvm::Value *sizeV = llvm::ConstantInt::get(Int64Ty, size);<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    switch (M->getStorageDuration()) {<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    case SD_FullExpression:<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      pushFullExprCleanup<CallLifetimeEnd>(NormalAndEHCleanup, Object, sizeV);<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      break;<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    case SD_Automatic:<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      EHStack.pushCleanup<CallLifetimeEnd>(static_cast<CleanupKind>(EHCleanup),<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+                                           Object, sizeV);<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      pushCleanupAfterFullExpr<CallLifetimeEnd>(NormalAndEHCleanup, Object,<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+                                                sizeV);<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      break;<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    default:<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      llvm_unreachable("unexpected storage duration for Lifetime markers");<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    }<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  }<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">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.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">   pushTemporaryCleanup(*this, M, E, Object);<u></u><u></u></p></div></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><div><p class="MsoNormal" style="margin-left:36.0pt">+  /// A cleanup to call @llvm.lifetime.end.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  class CallLifetimeEnd : public EHScopeStack::Cleanup {<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    llvm::Value *Addr;<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    llvm::Value *Size;<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  public:<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    CallLifetimeEnd(llvm::Value *addr, llvm::Value *size)<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      : Addr(addr), Size(size) {}<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    void Emit(CodeGenFunction &CGF, Flags flags) override {<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+      CGF.EmitLifetimeEnd(Size, Addr);<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+    }<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">+  };<u></u><u></u></p></div></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">Subclasses of EHScopeStack::Cleanup are generally declared in anonymous namespaces, according to this comment:<u></u><u></u></p></div><div><div><p class="MsoNormal" style="margin-left:36.0pt">  /// Cleanup implementations should generally be declared in an<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">  /// anonymous namespace.<u></u><u></u></p></div></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">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. :(<u></u><u></u></p></div></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p><div><p class="MsoNormal" style="margin-left:36.0pt">On Tue, Sep 23, 2014 at 9:06 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:36.0pt"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Ping</span><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"> </span><u></u><u></u></p><div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm"><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""> <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</span><u></u><u></u></p></div></div><div><div><p class="MsoNormal" style="margin-left:72.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:72.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)?</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:72.0pt"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";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:"Calibri","sans-serif";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:"Calibri","sans-serif";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 [<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</span><u></u><u></u></p><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p><div><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p><div><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p><div><p class="MsoNormal" style="margin-left:108.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:108.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:108.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"><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:108.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"><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:108.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"><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:108.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"><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:108.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"><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:108.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"><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:108.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:108.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:108.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:108.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"><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:108.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:108.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:108.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:108.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"><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:108.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"><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:108.0pt"> <u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:108.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:108.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:108.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"><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:108.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:108.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"><span style="font-size:11.0pt;font-family:"Courier New";color:#1f497d"> </span><u></u><u></u></p><p class="MsoNormal" style="margin-left:144.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:144.0pt"> <u></u><u></u></p><div><p class="MsoNormal" style="margin-left:144.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:144.0pt"> <u></u><u></u></p><div><p class="MsoNormal" style="margin-left:144.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:144.0pt">Hi All,<u></u><u></u></p><p class="MsoNormal" style="margin-left:144.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:144.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:144.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:144.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:144.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:144.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:144.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:144.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:144.0pt"> <u></u><u></u></p><p class="MsoNormal" style="margin-left:144.0pt">Cheers,<u></u><u></u></p><p class="MsoNormal" style="margin-left:144.0pt">--<u></u><u></u></p><p class="MsoNormal" style="margin-left:144.0pt">Arnaud A. de Grandmaison<u></u><u></u></p><p class="MsoNormal" style="margin-left:144.0pt"> <u></u><u></u></p></div></div><p class="MsoNormal" style="margin-bottom:12.0pt;margin-left:144.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:144.0pt"> <u></u><u></u></p></div></div></div></div></div></blockquote></div><p class="MsoNormal" style="margin-left:108.0pt"> <u></u><u></u></p></div></div></div></div></div></div><p class="MsoNormal" style="margin-right:0cm;margin-bottom:12.0pt;margin-left:36.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:36.0pt"><u></u> <u></u></p></div></div></div></div></div></blockquote></div><br></div>