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.
Thanks for the many comments so far,
-- adrian
0001-Test-that-we-emit-a-DW_AT_location-for-self-captured.patch
Description: Binary data
0001-Allocate-stack-storage-for-.block_descriptor-and-cap.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
