On Wed, Mar 13, 2013 at 11:56 AM, John McCall <[email protected]> wrote:
> On Mar 7, 2013, at 6:00 PM, Adrian Prantl <[email protected]> wrote: > > On Mar 6, 2013, at 1:47 PM, John McCall <[email protected]> wrote: > >> On Mar 1, 2013, at 10:15 AM, Adrian Prantl <[email protected]> wrote: > >>> On Feb 28, 2013, at 11:20 AM, John McCall <[email protected]> wrote: > >>>> On Feb 28, 2013, at 11:12 AM, Eric Christopher <[email protected]> > wrote: > >>>>> On Thu, Feb 28, 2013 at 11:06 AM, John McCall <[email protected]> > wrote: > >>>>> On Feb 27, 2013, at 3:17 PM, Eric Christopher <[email protected]> > wrote: > >>>>>> On Wed, Feb 27, 2013 at 11:49 AM, John McCall <[email protected]> > wrote: > >>>>>> On Feb 27, 2013, at 11:42 AM, Eric Christopher <[email protected]> > wrote: > >>>>>>> On Wed, Feb 27, 2013 at 11:39 AM, Adrian Prantl <[email protected]> > wrote: > >>>>>>> > >>>>>>> On Feb 27, 2013, at 11:31 AM, John McCall <[email protected]> > 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
