Sorry for the wait, this fell off my radar and I thought it landed. Looks good, please commit!
On Sat, Sep 27, 2014 at 4:00 PM, Arnaud A. de Grandmaison < [email protected]> wrote: > 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] <[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 > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
