Hi Reid,
Here is an updated patch with all your comments taken into account. 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 ? Thanks again for your review. Cheers, Arnaud From: Reid Kleckner [mailto:[email protected]] Sent: 27 September 2014 00:21 To: Arnaud De Grandmaison Cc: llvm cfe; Richard Smith Subject: Re: [PATCH] Emit lifetime start/end for unnamed objects --- take 3 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. ---- +CodeGenFunction::MoveDefferedCleanups(size_t OldLifetimeExtendedSize) { "Deferred", as you have it spelled elsewhere. + uint64_t size = + CGM.getDataLayout().getTypeStoreSize(ConvertTypeForMem(E->getType())); + bool useLifetimeMarkers = + (M->getStorageDuration() == SD_FullExpression || + M->getStorageDuration() == SD_Automatic) && + EmitLifetimeStart(size, Object); 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. +/// Emit a lifetime.begin marker if some criteria are satisfied. +/// \return true if a marker was emitted, false otherwise +bool CodeGenFunction::EmitLifetimeStart(uint64_t Size, llvm::Value *Addr) { + if (!shouldUseLifetimeMarkers(*this, Size)) + return false; + + llvm::Value *SizeV = llvm::ConstantInt::get(Int64Ty, Size); + llvm::Value *castAddr = Builder.CreateBitCast(Addr, Int8PtrTy); + Builder.CreateCall2(CGM.getLLVMLifetimeStartFn(), SizeV, castAddr) + ->setDoesNotThrow(); + return true; +} I think this can be simplified by returning SizeV or nullptr, and assigning the result to SizeForLifeTimeMarkers unconditionally. llvm::Value *Object = createReferenceTemporary(*this, M, E); + + uint64_t size = + CGM.getDataLayout().getTypeStoreSize(ConvertTypeForMem(E->getType())); + bool useLifetimeMarkers = + (M->getStorageDuration() == SD_FullExpression || + M->getStorageDuration() == SD_Automatic) && + EmitLifetimeStart(size, Object); + I think you should fold this into createReferenceTemporary(), and add an output parameter like SizeForLifeTimeMarkers that gets passed into pushTemporaryCleanup. if (auto *Var = dyn_cast<llvm::GlobalVariable>(Object)) { // If the temporary is a global and has a constant initializer, we may // have already initialized it. @@ -363,6 +371,24 @@ LValue CodeGenFunction::EmitMaterializeTemporaryExpr( } else { EmitAnyExprToMem(E, Object, Qualifiers(), /*IsInit*/true); } + + if (useLifetimeMarkers) { + llvm::Value *sizeV = llvm::ConstantInt::get(Int64Ty, size); + switch (M->getStorageDuration()) { + case SD_FullExpression: + pushFullExprCleanup<CallLifetimeEnd>(NormalAndEHCleanup, Object, sizeV); + break; + case SD_Automatic: + EHStack.pushCleanup<CallLifetimeEnd>(static_cast<CleanupKind>(EHCleanup), + Object, sizeV); + pushCleanupAfterFullExpr<CallLifetimeEnd>(NormalAndEHCleanup, Object, + sizeV); + break; + default: + llvm_unreachable("unexpected storage duration for Lifetime markers"); + } + } 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. + pushTemporaryCleanup(*this, M, E, Object); + /// A cleanup to call @llvm.lifetime.end. + class CallLifetimeEnd : public EHScopeStack::Cleanup { + llvm::Value *Addr; + llvm::Value *Size; + public: + CallLifetimeEnd(llvm::Value *addr, llvm::Value *size) + : Addr(addr), Size(size) {} + + void Emit(CodeGenFunction &CGF, Flags flags) override { + CGF.EmitLifetimeEnd(Size, Addr); + } + }; Subclasses of EHScopeStack::Cleanup are generally declared in anonymous namespaces, according to this comment: /// Cleanup implementations should generally be declared in an /// anonymous namespace. 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. :( On Tue, Sep 23, 2014 at 9:06 AM, Arnaud A. de Grandmaison <[email protected]> wrote: Ping From: [email protected] [mailto:[email protected]] On Behalf Of Arnaud A. de Grandmaison Sent: 18 September 2014 16:35 To: llvm cfe Subject: RE: [PATCH] Emit lifetime start/end for unnamed objects --- take 3 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)? Cheers, Arnaud From: David Blaikie [mailto:[email protected]] Sent: 16 September 2014 20:50 To: Arnaud De Grandmaison Cc: llvm cfe Subject: Re: [PATCH] Emit lifetime start/end for unnamed objects --- take 3 On Tue, Sep 16, 2014 at 1:29 AM, Arnaud A. de Grandmaison <[email protected]> wrote: Hi David, 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. 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 L 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. 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: | GCC | Clang | LT-32 | LT-16 | ===+=====+=======+=======+=======+ F1 | 432 | 608 | 608 | 400 | F2 | 432 | 640 | 640 | 432 | F3 | 384 | 368 | 368 | 192 | F4 | 320 | 400 | 400 | 224 | Stack size is expressed in bytes. GCC version 4.8 LT-32 is clang with this patch (default 32 bytes threshold for all temporaries). LT-16 is clang with this patch and a 16 bytes threshold for all temporaries. 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 ? 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. 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. 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) (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) - David Cheers, Arnaud From: David Blaikie [mailto:[email protected]] Sent: 16 September 2014 01:25 To: Arnaud De Grandmaison Cc: llvm cfe Subject: Re: [PATCH] Emit lifetime start/end for unnamed objects --- take 3 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? On Mon, Sep 15, 2014 at 4:05 PM, Arnaud A. de Grandmaison <[email protected]> wrote: Hi All, Please find attached a patch which teaches clang to emit lifetime.start / lifetime.end markers for unnamed temporary objects. This patch can greatly reduce the stack usage of some C++ code, where it is so easy to have short lived unnamed temporaries. 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. I would appreciate if someone knowledgeable with the lifetime extended temporaries & cleanup scopes could give a look to this patch. Cheers, -- Arnaud A. de Grandmaison _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
0001-Emit-lifetime.start-lifetime.end-markers-for-unnamed.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
