On Mon, Oct 29, 2012 at 6:07 PM, Dehao Chen <de...@google.com> wrote:
> Hi, Micheal,
>
> Thanks for explaining the design principle of debug info with gimple,
> now I can understand your concerns. And thanks for providing the
> patch.
>
> However, in some places after gimplification (e.g. tree-inline.c), we
> still updates the block/location info. And EXPR_LOCATION is still used
> widely after gimplification. Do you mean that in the long run, we'd
> want to remove all these?
Yes. Unfortunately many of the core inlining routines are also used by
the C++ frontend for template instantiation...
Richard.
> Thanks
> Dehao
>
> On Mon, Oct 29, 2012 at 9:49 AM, Dehao Chen <de...@google.com> wrote:
>> On Mon, Oct 29, 2012 at 9:10 AM, Richard Biener
>> <richard.guent...@gmail.com> wrote:
>>> On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <de...@google.com> wrote:
>>>> On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <m...@suse.de> wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, 29 Oct 2012, Richard Biener wrote:
>>>>>
>>>>>> > Well, you merely moved the bogus code to gimple-low.c. It is bogus
>>>>>> > because you unconditionally overwrite TREE_BLOCK of all operands (and
>>>>>> > all
>>>>
>>>> Emm, then in gimple-low.c, we should probably not unconditionally
>>>> overwrite gimple_block for stmts too?
>>>
>>> gimple stmts have no block before gimple-low.
>>>
>>>>>> > operands operands) with the statements block. I assume the unit-test
>>>>>> > you
>>>>>> > added shows the problem you were trying to fix?
>>>>>> >
>>>>>> > From the scan-assembler-no directive I'm not really sure what exactly
>>>>>> > the
>>>>>> > problem is you're seeing. Can you try to describe it with words for
>>>>>> > the
>>>>>> > example code? Which operands has no tree-block where it should have
>>>>>> > one,
>>>>>> > or which one has the wrong tree-block?
>>>>>>
>>>>>> trees without block could be an issue when we extract them to some other
>>>>>> statement (then without block), and move that to some random place.
>>>>>
>>>>> Even then it would be either the frontends job to set the block, or the
>>>>> the job of the pass that introduced the new expression, when there is a
>>>>> clearly sane candidate for the block.
>>>>
>>>> Please correct me if I'm wrong: front-end does not set blocks for
>>>> either stmt/expr. lower_stmt is the first place that block is set for
>>>> stmt, thus I think it should also be the first place to set block for
>>>> expr.
>>>
>>> No, only the block on the gimple stmt matters for gimple. You should not
>>> need to touch the operands block.
>>>
>>>>>
>>>>>> But the only issue should be that the stmt/expressions effective block
>>>>>> becomes a different one (the currently "active" one during expansion).
>>>>
>>>> I agree. Initially, both stmt and expressions is set to NULL block.
>>>> Whenever we update the stmt's block, we should also update the
>>>> expression's block, otherwise they will be inconsistent.
>>>
>>> I don't think so.
>>
>> Hi, Richard,
>>
>> Can you share some insights why you don't think we want to make stmt
>> and expr's block consistent?
>>
>>>
>>>>>
>>>>> Yep.
>>>>>
>>>>>> I don't see how we could end up generating too many block location
>>>>>> DIEs because of this.
>>>>
>>>> For the unittest I added, all the assembly code guarded by "if" are
>>>> should be within one lexical block, which is lock:
>>>>
>>>> LBB1
>>>> {
>>>> asm1
>>>> asm2
>>>> asm3
>>>> asm4
>>>> ....
>>>> }
>>>>
>>>> However, because some expression are with NULL lexical block, these
>>>> assembly codes are separated by several discontinual lexical block
>>>> which is like:
>>>>
>>>> LBB1
>>>> {
>>>> asm1
>>>> }
>>>> asm2
>>>
>>> asm2 should have gone into the currently open scope (similiar for what
>>> we do for line number information).
>>
>> In location_block patch, we changed this not to fall into the
>> currently open scope, because we want every insn to have correct
>> location/block with it. This is one of the reasons why we introduced
>> the location_block patch: it makes it easy to update block for
>> stmt/expr/phi_arg thus keeping a consistent location/block pair for
>> all instructions becomes possible.
>>
>> Yes, let location/block fall into the currently open scope/location
>> was a workaround before, but if we are able to make the location/block
>> pair consistent all through compilation, we should do that, right?
>>
>> Thanks,
>> Dehao
>>
>>>
>>>> LBB2
>>>> {
>>>> asm3
>>>> }
>>>> asm4
>>>> ....
>>>>
>>>>>
>>>>> And even if, I don't see what's the _problem_ with too many block
>>>>> locations, except bloat.
>>>>
>>>> Yes, bloat is the major problem.
>>>>
>>>> Thanks,
>>>> Dehao
>>>>
>>>>>
>>>>>
>>>>> Ciao,
>>>>> Michael.
