| Message ID | CAO2gOZU3HafUBWN00OvDqpM-qkjQ41nzXpsBMY_U=0Z51XZvRw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi,
On Fri, 26 Oct 2012, Dehao Chen wrote:
> 1. abandon the changes in cfgexpand.c
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
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?
Ciao,
Michael.
On Mon, Oct 29, 2012 at 2:51 PM, Michael Matz <matz@suse.de> wrote: > Hi, > > On Fri, 26 Oct 2012, Dehao Chen wrote: > >> 1. abandon the changes in cfgexpand.c > > 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 > 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. But the only issue should be that the stmt/expressions effective block becomes a different one (the currently "active" one during expansion). I don't see how we could end up generating too many block location DIEs because of this. Does the issue vanish when using -fno-tree-ter? And yes, I agree, the patch looks bogus. Richard. > > Ciao, > Michael.
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 > > 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. > But the only issue should be that the stmt/expressions effective block > becomes a different one (the currently "active" one during expansion). Yep. > I don't see how we could end up generating too many block location > DIEs because of this. And even if, I don't see what's the _problem_ with too many block locations, except bloat. Ciao, Michael.
On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@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? >> > 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. > >> 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. > > 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 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.
On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <dehao@google.com> wrote: > On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@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. >> >> 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). > 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.
On Mon, Oct 29, 2012 at 9:10 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <dehao@google.com> wrote: >> On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@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.
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 <dehao@google.com> wrote: > On Mon, Oct 29, 2012 at 9:10 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <dehao@google.com> wrote: >>> On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@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.
On Mon, Oct 29, 2012 at 6:07 PM, Dehao Chen <dehao@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 <dehao@google.com> wrote: >> On Mon, Oct 29, 2012 at 9:10 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <dehao@google.com> wrote: >>>> On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@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.
Ok. I'll test Micheal's patch, and send out the new patch soon. Thanks, Dehao
On Mon, Oct 29, 2012 at 05:10:10PM +0100, Richard Biener wrote: > On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <dehao@google.com> wrote: > > On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@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. And tree expressions don't have TREE_BLOCK before gimple-low either. So IMNSHO it is gimple-low.c that should set TREE_BLOCK of all the gimple stmts as well as all expression in the operands. It is not overwriting anything, no frontend sets TREE_BLOCK for any expression, the way frontends associate IL with BLOCKs is by putting them inside of BIND_EXPR/GIMPLE_BIND after gimplification, and it is gimple-low responsibility to set it. In 4.3 before tuples, it was solely gimple-low that set TREE_BLOCK initially. Before the location_t changes, again it was gimple-low that was the first setter of TREE_BLOCK, which was valid for all IS_EXPR_CODE_CLASS. So, IMNSHO gimple-low should merge location_t with block for all gimple stmts and all tree expressions used in its operands. It shouldn't be set on trees that can be shared, so say decls etc. should keep using just location_t's without associated block. So perhaps the right test for gimple-low setting of block is CAN_HAVE_LOCATION_P (exp) && !tree_node_can_be_shared (exp). Jakub
> And tree expressions don't have TREE_BLOCK before gimple-low either. > So IMNSHO it is gimple-low.c that should set TREE_BLOCK of all the gimple > stmts as well as all expression in the operands. It is not overwriting > anything, no frontend sets TREE_BLOCK for any expression, the way frontends > associate IL with BLOCKs is by putting them inside of BIND_EXPR/GIMPLE_BIND > after gimplification, and it is gimple-low responsibility to set it. > > In 4.3 before tuples, it was solely gimple-low that set TREE_BLOCK > initially. Before the location_t changes, again it was gimple-low that > was the first setter of TREE_BLOCK, which was valid for all > IS_EXPR_CODE_CLASS. So, IMNSHO gimple-low should merge location_t > with block for all gimple stmts and all tree expressions used in its > operands. It shouldn't be set on trees that can be shared, so > say decls etc. should keep using just location_t's without associated block. > So perhaps the right test for gimple-low setting of block is > CAN_HAVE_LOCATION_P (exp) && !tree_node_can_be_shared (exp). > > Jakub I Kind of like this idea. What do you guys think? Thanks, Dehao
On Tue, Oct 30, 2012 at 3:17 PM, Dehao Chen <dehao@google.com> wrote: >> And tree expressions don't have TREE_BLOCK before gimple-low either. >> So IMNSHO it is gimple-low.c that should set TREE_BLOCK of all the gimple >> stmts as well as all expression in the operands. It is not overwriting >> anything, no frontend sets TREE_BLOCK for any expression, the way frontends >> associate IL with BLOCKs is by putting them inside of BIND_EXPR/GIMPLE_BIND >> after gimplification, and it is gimple-low responsibility to set it. >> >> In 4.3 before tuples, it was solely gimple-low that set TREE_BLOCK >> initially. Before the location_t changes, again it was gimple-low that >> was the first setter of TREE_BLOCK, which was valid for all >> IS_EXPR_CODE_CLASS. So, IMNSHO gimple-low should merge location_t >> with block for all gimple stmts and all tree expressions used in its >> operands. It shouldn't be set on trees that can be shared, so >> say decls etc. should keep using just location_t's without associated block. >> So perhaps the right test for gimple-low setting of block is >> CAN_HAVE_LOCATION_P (exp) && !tree_node_can_be_shared (exp). >> >> Jakub > > I Kind of like this idea. What do you guys think? I question the need of BLOCK info on expression trees. If BLOCKs are relevant then the tree ends up referencing a declaration with a BLOCK as context, no? Thus, the case int tem, a; { int a; ... tem = a; } int b = tem + 5; where we may end up with gimple like b = a + 5; thus mixing two BLOCKs inside a stmt (and no expression trees to attach different BLOCKs) is no different from the case where we end up with expression trees. Thus my original question - why isn't a NULL BLOCK treated the same as UNKNOWN_LOCATION? Or rather, _why_ does Michas patch not work? Did you analyze the guality fails? Thanks, Richard. > Thanks, > Dehao
Hi, On Tue, 30 Oct 2012, Richard Biener wrote: > On Tue, Oct 30, 2012 at 3:17 PM, Dehao Chen <dehao@google.com> wrote: > >> And tree expressions don't have TREE_BLOCK before gimple-low either. > >> So IMNSHO it is gimple-low.c that should set TREE_BLOCK of all the gimple > >> stmts as well as all expression in the operands. That would be a step away from the ideal situation, so we rather should first analyze why the testcase fails with my patch. I expected some fallout and am actually surprised it's only one testcase :) What we should end up with in the ideal world is that we simply have no expressions in gimple (and hence no place to store any locations for them), except via gimple statements. > I question the need of BLOCK info on expression trees. If BLOCKs are > relevant then the tree ends up referencing a declaration with a BLOCK as > context, no? Thus, the case > > int tem, a; > { > int a; > ... > tem = a; > } > int b = tem + 5; > > where we may end up with gimple like > > b = a + 5; > > thus mixing two BLOCKs inside a stmt (and no expression trees to > attach different BLOCKs) is no different from the case where we end > up with expression trees. > > Thus my original question - why isn't a NULL BLOCK treated the same > as UNKNOWN_LOCATION? Since merging location and block a null BLOCK doesn't imply no location. It can very well have a location without a block. What we might want to imply is that a null BLOCK implies the BLOCK from the statement. But as you say, first we should find out why my patch breaks the one testcase. > Or rather, _why_ does Michas patch not work? Did you analyze the guality > fails? Ciao, Michael.
On Tue, Oct 30, 2012 at 3:49 PM, Michael Matz <matz@suse.de> wrote: > Hi, > > On Tue, 30 Oct 2012, Richard Biener wrote: > >> On Tue, Oct 30, 2012 at 3:17 PM, Dehao Chen <dehao@google.com> wrote: >> >> And tree expressions don't have TREE_BLOCK before gimple-low either. >> >> So IMNSHO it is gimple-low.c that should set TREE_BLOCK of all the gimple >> >> stmts as well as all expression in the operands. > > That would be a step away from the ideal situation, so we rather should > first analyze why the testcase fails with my patch. I expected some > fallout and am actually surprised it's only one testcase :) > > What we should end up with in the ideal world is that we simply have no > expressions in gimple (and hence no place to store any locations for > them), except via gimple statements. > >> I question the need of BLOCK info on expression trees. If BLOCKs are >> relevant then the tree ends up referencing a declaration with a BLOCK as >> context, no? Thus, the case >> >> int tem, a; >> { >> int a; >> ... >> tem = a; >> } >> int b = tem + 5; >> >> where we may end up with gimple like >> >> b = a + 5; >> >> thus mixing two BLOCKs inside a stmt (and no expression trees to >> attach different BLOCKs) is no different from the case where we end >> up with expression trees. >> >> Thus my original question - why isn't a NULL BLOCK treated the same >> as UNKNOWN_LOCATION? > > Since merging location and block a null BLOCK doesn't imply no location. > It can very well have a location without a block. What we might want to > imply is that a null BLOCK implies the BLOCK from the statement. But as > you say, first we should find out why my patch breaks the one testcase. Yes, I mean we happily leave the stmt line location the same if we have a stmt with UNKNOWN_LOCATION (thus it inherits that of the previous stmt), we should do the same with BLOCKs - if a stmt has a location with NULL BLOCK then it should inherit the block info from the previous stmt. Richard. >> Or rather, _why_ does Michas patch not work? Did you analyze the guality >> fails? > > > Ciao, > Michael.
On Tue, Oct 30, 2012 at 03:38:11PM +0100, Richard Biener wrote: > I question the need of BLOCK info on expression trees. If BLOCKs are > relevant then the tree ends up referencing a declaration with a BLOCK as > context, no? Thus, the case > > int tem, a; > { > int a; > ... > tem = a; > } > int b = tem + 5; > > where we may end up with gimple like > > b = a + 5; > > thus mixing two BLOCKs inside a stmt (and no expression trees to > attach different BLOCKs) is no different from the case where we end > up with expression trees. IMHO either we don't use locations at all on tree expressions (thus no source location nor block), or both. A source location has always an associated block it is present in. Of course for shared trees we can't put there any block, as it can appear anywhere. > Thus my original question - why isn't a NULL BLOCK treated the same > as UNKNOWN_LOCATION? Or rather, _why_ does Michas patch not work? > Did you analyze the guality fails? Micha's patch is degrading debug info quality. Whenever some expression has different source location from the source location of the gimple stmt, then assuming that other source location is from the same block is wrong, it could very well be from a different one. On the testcase that fails with Micha's patch, we have: [pr43479.c : 8:4] l_2 = l_1(D) + 1; [pr43479.c : 8:4] # DEBUG l => l_2 [pr43479.c : 10:9] # DEBUG h => n_3(D) [pr43479.c : 12:11] # DEBUG i => k_4(D) [pr43479.c : 13:8] k_5 = k_4(D) + 1; [pr43479.c : 13:8] # DEBUG k => k_5 [pr43479.c : 17:11] # DEBUG j => m_6(D) [pr43479.c : 18:8] m_7 = m_6(D) + 1; [pr43479.c : 18:8] # DEBUG m => m_7 [pr43479.c : 22:3] __asm__ __volatile__("" : : "r" k_5, "r" l_2); [pr43479.c : 23:3] __asm__ __volatile__("" : : "r" m_7, "r" n_3(D)); where line 8 is from the outer block in the source, line 10 from the middle block, line 12/13 from first innermost block, line 17/18 from second innermost block. But all of the l_2, k_5 and m_7 setters are TERed, so everything is emitted when expanding the two asm statements and with Micha's patch the block used is the block of the asm statement, while previously each TERed statement got its own block. I'd say either we should do the TREE_BLOCK setting on all non-shareable trees during gimple-low and clear the block (but then likely whole location?; it doesn't make sense to say in the debugger that something has certain source location when you can't print variables declared in that location) if copying expressions for use elsewhere, outside of the containing function. Or say during gimplification or gimple-low.c simply set t->exp.locus of all non-shareable expressions to UNKNOWN_LOCATION to make it clear we don't use it (wonder if that could affect debug info quality, perhaps not that much), but during expansion if creating trees from TERed stmts they need to be set back, or the current location/block adjusted accordingly. Jakub
On Tue, Oct 30, 2012 at 4:03 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Oct 30, 2012 at 03:38:11PM +0100, Richard Biener wrote: >> I question the need of BLOCK info on expression trees. If BLOCKs are >> relevant then the tree ends up referencing a declaration with a BLOCK as >> context, no? Thus, the case >> >> int tem, a; >> { >> int a; >> ... >> tem = a; >> } >> int b = tem + 5; >> >> where we may end up with gimple like >> >> b = a + 5; >> >> thus mixing two BLOCKs inside a stmt (and no expression trees to >> attach different BLOCKs) is no different from the case where we end >> up with expression trees. > > IMHO either we don't use locations at all on tree expressions (thus > no source location nor block), or both. A source location has always > an associated block it is present in. Of course for shared trees we > can't put there any block, as it can appear anywhere. > >> Thus my original question - why isn't a NULL BLOCK treated the same >> as UNKNOWN_LOCATION? Or rather, _why_ does Michas patch not work? >> Did you analyze the guality fails? > > Micha's patch is degrading debug info quality. Whenever some expression > has different source location from the source location of the gimple stmt, > then assuming that other source location is from the same block is wrong, > it could very well be from a different one. On the testcase that fails > with Micha's patch, we have: > [pr43479.c : 8:4] l_2 = l_1(D) + 1; > [pr43479.c : 8:4] # DEBUG l => l_2 > [pr43479.c : 10:9] # DEBUG h => n_3(D) > [pr43479.c : 12:11] # DEBUG i => k_4(D) > [pr43479.c : 13:8] k_5 = k_4(D) + 1; > [pr43479.c : 13:8] # DEBUG k => k_5 > [pr43479.c : 17:11] # DEBUG j => m_6(D) > [pr43479.c : 18:8] m_7 = m_6(D) + 1; > [pr43479.c : 18:8] # DEBUG m => m_7 > [pr43479.c : 22:3] __asm__ __volatile__("" : : "r" k_5, "r" l_2); > [pr43479.c : 23:3] __asm__ __volatile__("" : : "r" m_7, "r" n_3(D)); > where line 8 is from the outer block in the source, line 10 from the middle > block, line 12/13 from first innermost block, line 17/18 from second > innermost block. But all of the l_2, k_5 and m_7 setters are TERed, > so everything is emitted when expanding the two asm > statements and with Micha's patch the block used is the block of the asm > statement, while previously each TERed statement got its own block. > > I'd say either we should do the TREE_BLOCK setting on all non-shareable > trees during gimple-low and clear the block (but then likely whole > location?; it doesn't make sense to say in the debugger that something > has certain source location when you can't print variables declared in that > location) if copying expressions for use elsewhere, outside of the > containing function. Or say during gimplification or gimple-low.c > simply set t->exp.locus of all non-shareable expressions to UNKNOWN_LOCATION > to make it clear we don't use it (wonder if that could affect debug info > quality, perhaps not that much), but during expansion if creating trees > from TERed stmts they need to be set back, or the current location/block > adjusted accordingly. So maybe TER (well, those looking up the stmt) should pick the location from the TERed statement properly then? Richard. > Jakub
On Tue, Oct 30, 2012 at 04:15:38PM +0100, Richard Biener wrote: > So maybe TER (well, those looking up the stmt) should pick the location > from the TERed statement properly then? Perhaps, but Micha's patch doesn't do that. But in that case IMHO it still would help to set all expr locations to UNKNOWN_LOCATION during gimple lowering, to make it clear we ignore the locations. Jakub
On Tue, Oct 30, 2012 at 4:21 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Oct 30, 2012 at 04:15:38PM +0100, Richard Biener wrote: >> So maybe TER (well, those looking up the stmt) should pick the location >> from the TERed statement properly then? > > Perhaps, but Micha's patch doesn't do that. > But in that case IMHO it still would help to set all expr locations to > UNKNOWN_LOCATION during gimple lowering, to make it clear we ignore > the locations. Yes indeed. Richard. > Jakub
> I'd say either we should do the TREE_BLOCK setting on all non-shareable > trees during gimple-low and clear the block (but then likely whole > location?; it doesn't make sense to say in the debugger that something > has certain source location when you can't print variables declared in that > location) if copying expressions for use elsewhere, outside of the > containing function. Or say during gimplification or gimple-low.c > simply set t->exp.locus of all non-shareable expressions to UNKNOWN_LOCATION > to make it clear we don't use it (wonder if that could affect debug info > quality, perhaps not that much), but during expansion if creating trees > from TERed stmts they need to be set back, or the current location/block > adjusted accordingly. The debugger isn't the only consumer of debug info, and other tools might need a finer granularity than a GIMPLE location, so clearing EXPR_LOCATION to work around a debug info size issue seems very short-sighted (to say the least).
> The debugger isn't the only consumer of debug info, and other tools might need > a finer granularity than a GIMPLE location, so clearing EXPR_LOCATION to work > around a debug info size issue seems very short-sighted (to say the least). Hi, Eric, There might be some misunderstanding here. Clearing EXPR_LOCATION is not a work around to reduce debug size. It is aiming at making gcc implementation cleaner. And before we resetting it, we need to truely make sure nothing after gimple-low is dependent on it. Please let me know if you have other concerns. Thanks, Dehao > > -- > Eric Botcazou
Index: gcc/tree-eh.c =================================================================== --- gcc/tree-eh.c (revision 192809) +++ gcc/tree-eh.c (working copy) @@ -739,6 +739,7 @@ do_return_redirection (struct goto_queue_node *q, gimple_seq_add_seq (&q->repl_stmt, mod); x = gimple_build_goto (finlab); + gimple_set_location (x, q->location); gimple_seq_add_stmt (&q->repl_stmt, x); } @@ -758,6 +759,7 @@ do_goto_redirection (struct goto_queue_node *q, tr gimple_seq_add_seq (&q->repl_stmt, mod); x = gimple_build_goto (finlab); + gimple_set_location (x, q->location); gimple_seq_add_stmt (&q->repl_stmt, x); } @@ -857,6 +859,7 @@ frob_into_branch_around (gimple tp, eh_region regi if (!over) over = create_artificial_label (loc); x = gimple_build_goto (over); + gimple_set_location (x, loc); gimple_seq_add_stmt (&cleanup, x); } gimple_seq_add_seq (&eh_seq, cleanup); @@ -1085,6 +1088,7 @@ lower_try_finally_nofallthru (struct leh_state *st emit_post_landing_pad (&eh_seq, tf->region); x = gimple_build_goto (lab); + gimple_set_location (x, gimple_location (tf->try_finally_expr)); gimple_seq_add_stmt (&eh_seq, x); } } @@ -1223,6 +1227,7 @@ lower_try_finally_copy (struct leh_state *state, s tmp = lower_try_finally_fallthru_label (tf); x = gimple_build_goto (tmp); + gimple_set_location (x, tf_loc); gimple_seq_add_stmt (&new_stmt, x); } @@ -1395,6 +1400,7 @@ lower_try_finally_switch (struct leh_state *state, tmp = lower_try_finally_fallthru_label (tf); x = gimple_build_goto (tmp); + gimple_set_location (x, tf_loc); gimple_seq_add_stmt (&switch_body, x); } @@ -1423,6 +1429,7 @@ lower_try_finally_switch (struct leh_state *state, gimple_seq_add_stmt (&eh_seq, x); x = gimple_build_goto (finally_label); + gimple_set_location (x, tf_loc); gimple_seq_add_stmt (&eh_seq, x); tmp = build_int_cst (integer_type_node, eh_index); Index: gcc/gimple-low.c =================================================================== --- gcc/gimple-low.c (revision 192809) +++ gcc/gimple-low.c (working copy) @@ -331,7 +331,18 @@ lower_omp_directive (gimple_stmt_iterator *gsi, st gsi_next (gsi); } +/* Call back function to set the block for expr. */ +static tree +tree_set_block_r (tree *tp, int *walk_subtrees ATTRIBUTE_UNUSED, + void *data) +{ + tree block = (tree) data; + if (CAN_HAVE_LOCATION_P (*tp)) + TREE_SET_BLOCK (*tp, block); + return NULL_TREE; +} + /* Lower statement GSI. DATA is passed through the recursion. We try to track the fallthruness of statements and get rid of unreachable return statements in order to prevent the EH lowering pass from adding useless @@ -343,8 +354,11 @@ static void lower_stmt (gimple_stmt_iterator *gsi, struct lower_data *data) { gimple stmt = gsi_stmt (*gsi); + unsigned i; gimple_set_block (stmt, data->block); + for (i = 0; i < gimple_num_ops (stmt); i++) + walk_tree (gimple_op_ptr (stmt, i), tree_set_block_r, data->block, NULL); switch (gimple_code (stmt)) { Index: gcc/testsuite/g++.dg/debug/dwarf2/block.C =================================================================== --- gcc/testsuite/g++.dg/debug/dwarf2/block.C (revision 0) +++ gcc/testsuite/g++.dg/debug/dwarf2/block.C (revision 0) @@ -0,0 +1,29 @@ +// Compiler should not generate too many lexical blocks for this function. +// { dg-do compile { target { i?86-*-* x86_64-*-* } } } +// { dg-options "-O0 -fno-exceptions -g -dA" } + +union UElement { + void* pointer; + int integer; +}; +struct UColToken { + unsigned source; + unsigned char **rulesToParseHdl; +}; + +int uhash_hashTokens(const union UElement k) +{ + int hash = 0; + struct UColToken *key = (struct UColToken *)k.pointer; + if (key != 0) { + int len = (key->source & 0xFF000000)>>24; + int inc = ((len - 32) / 32) + 1; + const unsigned char *p = (key->source & 0x00FFFFFF) + + *(key->rulesToParseHdl); + const unsigned char *limit = p + len; + hash = *p + *limit; + } + return hash; +} + +// { dg-final { scan-assembler-not "LBB10" } }
Hi, I've updated the patch: 1. abandon the changes in cfgexpand.c 2. set the block for trees when lowering gimple stmt. 3. add a unittest. However, this patch will trigger two lto bug when asserting LTO_NO_PREVAIL for TREE_CHAIN. After debugging for a while, I found that the problem was also there even without the patch. This patch just reveal the problem by moving a decl into cache so that it will be checked. As I'm not familiar with LTO, not quite sure what the root problem is. Can anyone help take a look? Thanks, Dehao gcc/ChangeLog: 2012-10-25 Dehao Chen <dehao@google.com> * tree-eh.c (do_return_redirection): Set location for jump statement. (do_goto_redirection): Likewise. (frob_into_branch_around): Likewise. (lower_try_finally_nofallthru): Likewise. (lower_try_finally_copy): Likewise. (lower_try_finally_switch): Likewise. * gimple-low.c (tree_set_block_r): New callback function. (lower_stmt): Set block for tested expr. gcc/testsuite/ChangeLog: 2012-10-25 Dehao Chen <dehao@google.com> * g++.dg/debug/dwarf2/block.C: New testcase.