[PATCH] Allocate stack storage for .block_descriptor and captured self.

[Eric Christopher]

On Wed, Mar 13, 2013 at 11:56 AM, John McCall <rjmccall at apple.com> wrote:

> On Mar 7, 2013, at 6:00 PM, Adrian Prantl <aprantl at apple.com> wrote:
> > On Mar 6, 2013, at 1:47 PM, John McCall <rjmccall at apple.com> wrote:
> >> On Mar 1, 2013, at 10:15 AM, Adrian Prantl <aprantl at apple.com> wrote:
> >>> On Feb 28, 2013, at 11:20 AM, John McCall <rjmccall at apple.com> wrote:
> >>>> On Feb 28, 2013, at 11:12 AM, Eric Christopher <echristo at gmail.com>
> wrote:
> >>>>> On Thu, Feb 28, 2013 at 11:06 AM, John McCall <rjmccall at apple.com>
> wrote:
> >>>>> On Feb 27, 2013, at 3:17 PM, Eric Christopher <echristo at gmail.com>
> wrote:
> >>>>>> On Wed, Feb 27, 2013 at 11:49 AM, John McCall <rjmccall at apple.com>
> wrote:
> >>>>>> On Feb 27, 2013, at 11:42 AM, Eric Christopher <echristo at gmail.com>
> wrote:
> >>>>>>> On Wed, Feb 27, 2013 at 11:39 AM, Adrian Prantl <aprantl at apple.com>
> wrote:
> >>>>>>>
> >>>>>>> On Feb 27, 2013, at 11:31 AM, John McCall <rjmccall at apple.com>
> wrote:
> >>>>>>>> Okay, you're saying that the value is actually no longer live at
> all at this point in the function?  It seems reasonable to lose track of
> the debug info then, although we should be leaving behind a marker in the
> DWARF that says the value is unavailable.
> >>>>>>>>
> >>>>>>>> If we want to make stronger guarantees in -O0 for purposes of
> debugging — and I think that's reasonable — then throwing the value in an
> alloca is acceptable.
> >>>>>>>
> >>>>>>> To clarify: Are you suggesting to only generate the alloca at -O0,
> or are you comfortable with it as it is?
> >>>>>>>
> >>>>>>> If the value isn't live past that spot I'm more comfortable with
> dropping the debug info there rather than changing the generated code to
> keep the value live through the end of the function.
> >>>>>>
> >>>>>> Purely out of attachment to the principle that debug info shouldn't
> change the code?
> >>>>>>
> >>>>>>
> >>>>>> Pretty much.
> >>>>>>
> >>>>>> Not losing information has intrinsic value in a debug build.  If we
> need to emit slightly different code in order to force a value to stay live
> and thus improve the debugging experience, then so be it.
> >>>>>>
> >>>>>>
> >>>>>> Agreed that making the experience better is desirable, but it can
> make debugging a problem more difficult if the code changes when you turn
> on debugging symbols.
> >>>>>
> >>>>> Ah, I see your point;  not doing the alloca could slide stack frames
> around.
> >>>>>
> >>>>> Alright, I agree with emitting it in all -O0 builds.
> >>>>>
> >>>>> Thought if optimization should fix it then perhaps all builds? :)
> >>>>
> >>>> I don't see any point in creating it just for mem2reg to trivially
> destroy. :)
> >>>>
> >>>>> That said I'll remove the objection to the allocas. We'll need to
> fix the alloca problem at some point, but making poor Adrian do it right
> now for this bug when we've got other workarounds already in the source
> base seems a bit mean.
> >>>>
> >>>> Well, if the value really isn't live anymore, then I'm not sure what
> the supposed alloca problem is, other than needing to leave breadcrumbs to
> say that the value isn't available at this point in the function.  We
> definitely don't want regalloc to be keeping values live just for debug
> info!
> >>>
> >>> FYI: this is what the patch looks like if output the alloca only at
> -O0.
> >>
> >> +  if (CGM.getCodeGenOpts().OptimizationLevel == 0)
> >> +    blockAddr = Builder.CreateLoad(blockAddr);
> >>
> >> No.  The kind of value in LocalDeclMap should not vary according to
> >> target optimization level.
> >
> > Following yours and Eric’s suggestions I got rid of most
> OptmiziationLevel-dependent code.
> >
> >> What you should do is:
> >> - under -O0, always emit an alloca for the block context parameter and
> store
> >>  the block context into it, and
> >> - under -g, tie the debug info the block context parameter to the
> alloca, if it
> >>  exists, and otherwise to the raw argument value.
> >
> > That works well for the block context parameter.
> >>
> >> +      if (CGM.getCodeGenOpts().OptimizationLevel == 0) {
> >> +    llvm::Value *selfAddr =
> >>
> >> Don't do this.  If you need to re-indent, re-indent.
> >
> > Tabs vs spaces. I finally fixed my .emacs.
> >
> >> +// RUN: %clang_cc1 %s -O0 -fblocks -triple x86_64-apple-darwin
> -emit-llvm -o - | FileCheck %s
> >>
> >> I'm confused, because that's the default.
> >
> > You’re correct, the -O0 is redundant.
> >
> >> +// RUN: %clang -fblocks -S -g -fverbose-asm -triple
> x86_64-apple-darwin -o - %s | FileCheck %s
> >>
> >> Like Eric mentioned, tests that need to check backend output shouldn't
> go
> >> in the clang test suite.  Change the test to check IR output or move it
> to
> >> LLVM.
> >
> >
> > I split the benchmark in two: One part tests ObjC -> llvm IR generation
> and another one tests llvm IR -> Dwarf.
>
> The clang side of this looks fine, except you don't seem to be actually
> testing for the presence of your debug intrinsic?
>
> With that, and if Eric's okay with the LLVM side, go ahead.
>

Agreed. Same comments. The LLVM test is fine.

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130313/ccb9ba94/attachment.html>