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?
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.
