Patch submissions and discussion of these patches

Dehao Chen | 1 Aug 2012 07:35

Picon

[PATCH] Combine location with block using block_locations

Hi,

This patch:

* Integrates location with block into an integrated index.
* Removes gimple->gsbase.block and tree->exp.block fields.
* Updates inline/clone as well as tree liveness analysis to ensure the
associated blocks are updated correctly.

With this patch, the association between source location and its block
are greatly enhanced, which produces much better inline stack in the
debug info.

Bootstrapped and regression tested on x86.

OK for trunk?

Thanks,
Dehao

You can also find the patch in http://codereview.appspot.com/6454077

gcc/ChangeLog
2012-08-01  Dehao Chen  <dehao <at> google.com>

	* toplev.c (general_init): Init block_locations.
	* tree.c (tree_set_block): New.
	(tree_block): Change to use LOCATION_BLOCK.
	* tree.h (TREE_SET_BLOCK): New.
	* final.c (reemit_insn_block_notes): Change to use LOCATION_BLOCK.

(Continue reading)

Richard Guenther | 1 Aug 2012 20:07

Picon

Re: [PATCH] Combine location with block using block_locations

On Wed, Aug 1, 2012 at 7:35 AM, Dehao Chen <dehao <at> google.com> wrote:
> Hi,
>
> This patch:
>
> * Integrates location with block into an integrated index.
> * Removes gimple->gsbase.block and tree->exp.block fields.
> * Updates inline/clone as well as tree liveness analysis to ensure the
> associated blocks are updated correctly.
>
> With this patch, the association between source location and its block
> are greatly enhanced, which produces much better inline stack in the
> debug info.
>
> Bootstrapped and regression tested on x86.
>
> OK for trunk?

Nice.  But the LTO changes mean that you simply drop all BLOCK
associations on the floor ...
they at least look very incomplete.  Did you actually run LTO tests?

lto/ has its own ChangeLog and I wonder why no frontends are affected
by this patch?

Richard.

> Thanks,
> Dehao
>

(Continue reading)

Dehao Chen | 2 Aug 2012 05:23

Picon

Re: [PATCH] Combine location with block using block_locations

On Thu, Aug 2, 2012 at 2:07 AM, Richard Guenther
<richard.guenther <at> gmail.com> wrote:
> On Wed, Aug 1, 2012 at 7:35 AM, Dehao Chen <dehao <at> google.com> wrote:
>> Hi,
>>
>> This patch:
>>
>> * Integrates location with block into an integrated index.
>> * Removes gimple->gsbase.block and tree->exp.block fields.
>> * Updates inline/clone as well as tree liveness analysis to ensure the
>> associated blocks are updated correctly.
>>
>> With this patch, the association between source location and its block
>> are greatly enhanced, which produces much better inline stack in the
>> debug info.
>>
>> Bootstrapped and regression tested on x86.
>>
>> OK for trunk?
>
> Nice.  But the LTO changes mean that you simply drop all BLOCK
> associations on the floor ...

Why? I've invoked TREE_SET_BLOCK in tree-streamer-in.c to read in the
block info. So it should at least provide the same info as the
original impl.

> they at least look very incomplete.  Did you actually run LTO tests?

Thanks for the reminder, I've added the following change to this patch:

(Continue reading)

Richard Guenther | 2 Aug 2012 12:05

Picon

Re: [PATCH] Combine location with block using block_locations

On Thu, Aug 2, 2012 at 5:23 AM, Dehao Chen <dehao <at> google.com> wrote:
> On Thu, Aug 2, 2012 at 2:07 AM, Richard Guenther
> <richard.guenther <at> gmail.com> wrote:
>> On Wed, Aug 1, 2012 at 7:35 AM, Dehao Chen <dehao <at> google.com> wrote:
>>> Hi,
>>>
>>> This patch:
>>>
>>> * Integrates location with block into an integrated index.
>>> * Removes gimple->gsbase.block and tree->exp.block fields.
>>> * Updates inline/clone as well as tree liveness analysis to ensure the
>>> associated blocks are updated correctly.
>>>
>>> With this patch, the association between source location and its block
>>> are greatly enhanced, which produces much better inline stack in the
>>> debug info.
>>>
>>> Bootstrapped and regression tested on x86.
>>>
>>> OK for trunk?
>>
>> Nice.  But the LTO changes mean that you simply drop all BLOCK
>> associations on the floor ...
>
> Why? I've invoked TREE_SET_BLOCK in tree-streamer-in.c to read in the
> block info. So it should at least provide the same info as the
> original impl.
>
>> they at least look very incomplete.  Did you actually run LTO tests?
>

(Continue reading)

Dehao Chen | 6 Aug 2012 20:35

Picon

Re: [PATCH] Combine location with block using block_locations

On Thu, Aug 2, 2012 at 6:05 PM, Richard Guenther
<richard.guenther <at> gmail.com> wrote:
> On Thu, Aug 2, 2012 at 5:23 AM, Dehao Chen <dehao <at> google.com> wrote:
>> On Thu, Aug 2, 2012 at 2:07 AM, Richard Guenther
>> <richard.guenther <at> gmail.com> wrote:
>>> On Wed, Aug 1, 2012 at 7:35 AM, Dehao Chen <dehao <at> google.com> wrote:
>>>> Hi,
>>>>
>>>> This patch:
>>>>
>>>> * Integrates location with block into an integrated index.
>>>> * Removes gimple->gsbase.block and tree->exp.block fields.
>>>> * Updates inline/clone as well as tree liveness analysis to ensure the
>>>> associated blocks are updated correctly.
>>>>
>>>> With this patch, the association between source location and its block
>>>> are greatly enhanced, which produces much better inline stack in the
>>>> debug info.
>>>>
>>>> Bootstrapped and regression tested on x86.
>>>>
>>>> OK for trunk?
>>>
>>> Nice.  But the LTO changes mean that you simply drop all BLOCK
>>> associations on the floor ...
>>
>> Why? I've invoked TREE_SET_BLOCK in tree-streamer-in.c to read in the
>> block info. So it should at least provide the same info as the
>> original impl.
>>

(Continue reading)

Richard Guenther | 7 Aug 2012 17:15

Picon

Re: [PATCH] Combine location with block using block_locations

On Mon, Aug 6, 2012 at 8:35 PM, Dehao Chen <dehao <at> google.com> wrote:
> On Thu, Aug 2, 2012 at 6:05 PM, Richard Guenther
> <richard.guenther <at> gmail.com> wrote:
>> On Thu, Aug 2, 2012 at 5:23 AM, Dehao Chen <dehao <at> google.com> wrote:
>>> On Thu, Aug 2, 2012 at 2:07 AM, Richard Guenther
>>> <richard.guenther <at> gmail.com> wrote:
>>>> On Wed, Aug 1, 2012 at 7:35 AM, Dehao Chen <dehao <at> google.com> wrote:
>>>>> Hi,
>>>>>
>>>>> This patch:
>>>>>
>>>>> * Integrates location with block into an integrated index.
>>>>> * Removes gimple->gsbase.block and tree->exp.block fields.
>>>>> * Updates inline/clone as well as tree liveness analysis to ensure the
>>>>> associated blocks are updated correctly.
>>>>>
>>>>> With this patch, the association between source location and its block
>>>>> are greatly enhanced, which produces much better inline stack in the
>>>>> debug info.
>>>>>
>>>>> Bootstrapped and regression tested on x86.
>>>>>
>>>>> OK for trunk?
>>>>
>>>> Nice.  But the LTO changes mean that you simply drop all BLOCK
>>>> associations on the floor ...
>>>
>>> Why? I've invoked TREE_SET_BLOCK in tree-streamer-in.c to read in the
>>> block info. So it should at least provide the same info as the
>>> original impl.

(Continue reading)

Dehao Chen | 7 Aug 2012 17:51

Picon

Re: [PATCH] Combine location with block using block_locations

Reattaching the ChangeLog with the patch.

Bootstrapped and passed regression tests on x86_64

gcc/ChangeLog:

2012-08-01  Dehao Chen  <dehao <at> google.com>

	* toplev.c (general_init): Init block_locations.
	* tree.c (tree_set_block): New.
	(tree_block): Change to use LOCATION_BLOCK.
	* tree.h (TREE_SET_BLOCK): New.
	* final.c (reemit_insn_block_notes): Change to use LOCATION_BLOCK.
	(final_start_function): Likewise.
	* input.c (expand_location_1): Likewise.
	* input.h (LOCATION_LOCUS): New.
	(LOCATION_BLOCK): New.
	(IS_UNKNOWN_LOCATION): New.
	* fold-const.c (expr_location_or): Change to use new location.
	* reorg.c (emit_delay_sequence): Likewise.
	(try_merge_delay_insns): Likewise.
	* modulo-sched.c (dump_insn_location): Likewise.
	* lto-streamer-out.c (lto_output_location_bitpack): Likewise.
	* jump.c (rtx_renumbered_equal_p): Likewise.
	* ifcvt.c (noce_try_move): Likewise.
	(noce_try_store_flag): Likewise.
	(noce_try_store_flag_constants): Likewise.
	(noce_try_addcc): Likewise.
	(noce_try_store_flag_mask): Likewise.
	(noce_try_cmove): Likewise.

(Continue reading)

Dodji Seketeli | 13 Aug 2012 20:34

Picon

Re: [PATCH] Combine location with block using block_locations

Hello Dehao,

I have mostly cosmetic comments to make about the libcpp parts.

Dehao Chen <dehao <at> google.com> writes:

> Index: libcpp/include/line-map.h
> ===================================================================
> *** libcpp/include/line-map.h	(revision 189835)
> --- libcpp/include/line-map.h	(working copy)
> *************** struct GTY(()) line_map_ordinary {
> *** 89,95 ****
>

Just to sum things up, here is what I understand from the libcpp
changes.

The integer space is now segmented like this:

    X  A  B  C
    [        ]  <--- integer space of source_location

C is the smallest possible source_location and X is the biggest
possible.

From X to A, we have instances of source_location encoded in an ad-hoc
client specific (from the point of view of libcpp/line-map) map.

From A to B, we have instances of source_location encoded in macro maps.

(Continue reading)

Dehao Chen | 14 Aug 2012 06:57

Picon

Re: [PATCH] Combine location with block using block_locations

Hi, Dodji,

Thanks a lot for the review. I've updated the patch, which is attached.

Passed bootstrap and all regression tests.

Thanks,
Dehao

gcc/ChangeLog
2012-08-01  Dehao Chen  <dehao <at> google.com>

	* toplev.c (general_init): Init block_locations.
	* tree.c (tree_set_block): New.
	(tree_block): Change to use LOCATION_BLOCK.
	* tree.h (TREE_SET_BLOCK): New.
	* final.c (reemit_insn_block_notes): Change to use LOCATION_BLOCK.
	(final_start_function): Likewise.
	* input.c (expand_location_1): Likewise.
	* input.h (LOCATION_LOCUS): New.
	(LOCATION_BLOCK): New.
	(IS_UNKNOWN_LOCATION): New.
	* fold-const.c (expr_location_or): Change to use new location.
	* reorg.c (emit_delay_sequence): Likewise.
	(try_merge_delay_insns): Likewise.
	* modulo-sched.c (dump_insn_location): Likewise.
	* lto-streamer-out.c (lto_output_location_bitpack): Likewise.
	* jump.c (rtx_renumbered_equal_p): Likewise.
	* ifcvt.c (noce_try_move): Likewise.
	(noce_try_store_flag): Likewise.

(Continue reading)

Dodji Seketeli | 14 Aug 2012 09:56

Picon

Re: [PATCH] Combine location with block using block_locations

Dehao Chen <dehao <at> google.com> writes:

> Index: libcpp/line-map.c

[...]

> + /* Data structure to associate an arbitrary data to a source location.  */
> + struct location_adhoc_data {
> +   source_location locus;
> +   void *data;
> + };
> +
> + /* The following data structure encodes a location with some adhoc data,
> +    and map it to a new unsigned integer, and replace it with the original

I think you should remove the words "it with".

> +    location to represent the mapping.

So it should read (so far):

    The following data structure encodes a location with some adhoc data
    and maps it to a new unsigned integer (called an adhoc location)
    that replaces the original location to represent the mapping.

> +
> +    The new adhoc_loc uses the highest bit as the enabling bit, i.e. if the
> +    highest bit is 1, then the number is adhoc_loc. Otherwise, it serves as
> +    the original location. Once identified as the adhoc_loc, the lower 31
> +    bits of the integer is used to index to the location_adhoc_data array,

(Continue reading)

Dehao Chen | 14 Aug 2012 19:13

Picon

Re: [PATCH] Combine location with block using block_locations

Hi, Dodji,

Thanks for the review. I've fixed all the addressed issues. I'm
attaching the related changes:

Thanks,
Dehao

libcpp/ChangeLog:
2012-08-01  Dehao Chen  <dehao <at> google.com>

	* include/line-map.h (MAX_SOURCE_LOCATION): New value.
	(location_adhoc_data_init): New.
	(location_adhoc_data_fini): New.
	(get_combined_adhoc_loc): New.
	(get_data_from_adhoc_loc): New.
	(get_location_from_adhoc_loc): New.
	(COMBINE_LOCATION_DATA): New.
	(IS_ADHOC_LOC): New.
	(expanded_location): New field.
	* line-map.c (location_adhoc_data): New.
	(location_adhoc_data_htab): New.
	(curr_adhoc_loc): New.
	(location_adhoc_data): New.
	(allocated_location_adhoc_data): New.
	(location_adhoc_data_hash): New.
	(location_adhoc_data_eq): New.
	(location_adhoc_data_update): New.
	(get_combined_adhoc_loc): New.
	(get_data_from_adhoc_loc): New.

(Continue reading)

Dehao Chen | 20 Aug 2012 03:18

Picon

Re: [PATCH] Combine location with block using block_locations

ping....

Thanks,
Dehao

On Tue, Aug 14, 2012 at 10:13 AM, Dehao Chen <dehao <at> google.com> wrote:
> Hi, Dodji,
>
> Thanks for the review. I've fixed all the addressed issues. I'm
> attaching the related changes:
>
> Thanks,
> Dehao
>
> libcpp/ChangeLog:
> 2012-08-01  Dehao Chen  <dehao <at> google.com>
>
>         * include/line-map.h (MAX_SOURCE_LOCATION): New value.
>         (location_adhoc_data_init): New.
>         (location_adhoc_data_fini): New.
>         (get_combined_adhoc_loc): New.
>         (get_data_from_adhoc_loc): New.
>         (get_location_from_adhoc_loc): New.
>         (COMBINE_LOCATION_DATA): New.
>         (IS_ADHOC_LOC): New.
>         (expanded_location): New field.
>         * line-map.c (location_adhoc_data): New.
>         (location_adhoc_data_htab): New.
>         (curr_adhoc_loc): New.
>         (location_adhoc_data): New.

(Continue reading)

Richard Guenther | 21 Aug 2012 15:25

Picon

Re: [PATCH] Combine location with block using block_locations

On Mon, Aug 20, 2012 at 3:18 AM, Dehao Chen <dehao <at> google.com> wrote:
> ping....

Conceptually I like the change.  Can a libcpp maintainer please have a 2nd
look?

Dehao, did you do any compile-time and memory-usage benchmarks?

Thanks,
Richard.

> Thanks,
> Dehao
>
> On Tue, Aug 14, 2012 at 10:13 AM, Dehao Chen <dehao <at> google.com> wrote:
>> Hi, Dodji,
>>
>> Thanks for the review. I've fixed all the addressed issues. I'm
>> attaching the related changes:
>>
>> Thanks,
>> Dehao
>>
>> libcpp/ChangeLog:
>> 2012-08-01  Dehao Chen  <dehao <at> google.com>
>>
>>         * include/line-map.h (MAX_SOURCE_LOCATION): New value.
>>         (location_adhoc_data_init): New.
>>         (location_adhoc_data_fini): New.
>>         (get_combined_adhoc_loc): New.

(Continue reading)

Return

Return to gmane.comp.gcc.patches.

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane