On Mon, Oct 29, 2012 at 9:10 AM, Richard Biener
<[email protected]> wrote:
> On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <[email protected]> wrote:
>> On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <[email protected]> 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.
