[PATCH] Emit lifetime start/end for unnamed objects --- take 3
Reid Kleckner
rnk at google.com
Fri Sep 26 15:21:07 PDT 2014
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 <
arnaud.degrandmaison at arm.com> wrote:
> Ping
>
>
>
> *From:* cfe-commits-bounces at cs.uiuc.edu [mailto:
> cfe-commits-bounces at cs.uiuc.edu] *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:dblaikie at gmail.com <dblaikie at gmail.com>]
> *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 <
> arnaud.degrandmaison at arm.com> 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:dblaikie at gmail.com]
> *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 <
> arnaud.degrandmaison at arm.com> 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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140926/a52e88bc/attachment.html>
More information about the cfe-commits
mailing list