[PATCH] Emit lifetime start/end for unnamed objects --- take 3

Tue Oct 7 17:12:41 PDT 2014

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 <
arnaud.degrandmaison at arm.com> 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:rnk at google.com]
> *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 <
> 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/20141007/b65d260d/attachment.html>