| Message ID | 20110404014451.GA16239@nightcrawler |
|---|---|
| State | New |
| Headers | show |
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 04/03/11 19:44, Nathan Froyd wrote: > The patch below converts gcse.c:canon_modify_mem_list to hold VECs > instead of EXPR_LIST rtxes. I am ambivalent about the use of VECs in > canon_modify_mem_list; they will waste some memory compared to the > linked list scheme present before, though I'm not sure how much. It > would depend on the average chain length, etc. I'm happy to use an > linked list datastructure instead, allocated out of an > alloc_pool (better statistics!) or even gcse's private obstack if folks > think that would be better. Moving things out of GC memory and > eliminating a use of EXPR_LIST has to be considered a good thing, > though... I've got no strong opinions on all this stuff -- except that blindly moving stuff out of GC memory isn't necessarily a good thing. It really depends on the lifetime of the objects. Assuming the memory list stuff in gcse doesn't have a lifetime outside of GCSE and is thus easily tracked, then I've got no fundamental objection to the change. > > Doing this required addressing an odd little comment in > record_last_mem_set_info: > > if (CALL_P (insn)) > { > /* Note that traversals of this loop (other than for free-ing) > will break after encountering a CALL_INSN. So, there's no > need to insert a pair of items, as canon_list_insert does. */ > canon_modify_mem_list[bb] = > alloc_INSN_LIST (insn, canon_modify_mem_list[bb]); > bitmap_set_bit (blocks_with_calls, bb); > } > > This is all well and good, except that the only real traversal of > canon_modify_mem_list (compute_transp) doesn't check for CALL_INSNs. It's possible (likely?) the implementation changed over time and the comment wasn't properly updated. Unfortunate, but it does happen. > + canon_modify_mem_list = GCNEWVEC (VEC(modify_pair,heap) *, > + last_basic_block); nit; You're missing some whitespace here (after the VEC). OK. Please install, Thanks, Jeff -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJNmgdkAAoJEBRtltQi2kC7QagH/AkchggJ4C7SU2AasolDyQqn tcQowd5zBmiYFujY9+UgIL6Wh6AVU/Ls452c96MVKKWcDi8kIW0y3tzlls5yYbKW /XtvuzPU9zhya672mjTNktD3mPFj4qKtAO7PsjCh375uvkSknSXCAP9B5O9nQPbR BdaaAHv4gLgrpIokFTxk5455/7BGMCNJ0/O91PR4Jyithc2wZsz6Me4AFg+aMZG/ t+Vq7+6D5kALiXrrn2UNzrGefE6i6HdbacP6drOaDI1XNmI8Se4NgiE/JQfkvKty 1i4MVGW2IJrMax7fCKLhIRErQxEgfGQVfOLk5WkQXSzxfvILLu1bkdTTLKTr6t0= =tTk7 -----END PGP SIGNATURE-----
On Mon, Apr 04, 2011 at 12:01:09PM -0600, Jeff Law wrote: > > + canon_modify_mem_list = GCNEWVEC (VEC(modify_pair,heap) *, > > + last_basic_block); > nit; You're missing some whitespace here (after the VEC). This doesn't seem to be a hard-and-fast policy; all of the VEC code I remember writing or looking at recently has no spaces, and I checked the patch in on that basis. However, running grep indicates that we have a profusion of styles... -Nathan
On Tue, 2011-04-05 at 04:44 -0700, Nathan Froyd wrote: > On Mon, Apr 04, 2011 at 12:01:09PM -0600, Jeff Law wrote: > > > + canon_modify_mem_list = GCNEWVEC (VEC(modify_pair,heap) *, > > > + last_basic_block); > > nit; You're missing some whitespace here (after the VEC). > > This doesn't seem to be a hard-and-fast policy; all of the VEC code I > remember writing or looking at recently has no spaces, and I checked the > patch in on that basis. However, running grep indicates that we have a > profusion of styles... I think the style guide is quite clear on this: there should be a space there as Jeff says. The fact that some code has crept in with other styles doesn't make them right, or give justification for ignoring the style guide. R.
On Tue, Apr 05, 2011 at 01:22:39PM +0100, Richard Earnshaw wrote: > On Tue, 2011-04-05 at 04:44 -0700, Nathan Froyd wrote: > > > nit; You're missing some whitespace here (after the VEC). > > > > This doesn't seem to be a hard-and-fast policy; all of the VEC code I > > remember writing or looking at recently has no spaces, and I checked the > > patch in on that basis. However, running grep indicates that we have a > > profusion of styles... > > I think the style guide is quite clear on this: there should be a space > there as Jeff says. The fact that some code has crept in with other > styles doesn't make them right, or give justification for ignoring the > style guide. Would you like a patch for the hundreds of instances without spaces? Certainly vec.h never uses spaces; I thought this was simply The Way Things Were. -Nathan
On Tue, 2011-04-05 at 05:30 -0700, Nathan Froyd wrote: > On Tue, Apr 05, 2011 at 01:22:39PM +0100, Richard Earnshaw wrote: > > On Tue, 2011-04-05 at 04:44 -0700, Nathan Froyd wrote: > > > > nit; You're missing some whitespace here (after the VEC). > > > > > > This doesn't seem to be a hard-and-fast policy; all of the VEC code I > > > remember writing or looking at recently has no spaces, and I checked the > > > patch in on that basis. However, running grep indicates that we have a > > > profusion of styles... > > > > I think the style guide is quite clear on this: there should be a space > > there as Jeff says. The fact that some code has crept in with other > > styles doesn't make them right, or give justification for ignoring the > > style guide. > > Would you like a patch for the hundreds of instances without spaces? > > Certainly vec.h never uses spaces; I thought this was simply The Way > Things Were. > > -Nathan > IMO, rototils are generally pointless. If you are fixing code nearby then yes, fix the formatting issues. Otherwise, just don't exacerbate the problem, or we'll reach the point where a rototil really does become necessary. R.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 04/05/11 06:55, Richard Earnshaw wrote: > > On Tue, 2011-04-05 at 05:30 -0700, Nathan Froyd wrote: >> On Tue, Apr 05, 2011 at 01:22:39PM +0100, Richard Earnshaw wrote: >>> On Tue, 2011-04-05 at 04:44 -0700, Nathan Froyd wrote: >>>>> nit; You're missing some whitespace here (after the VEC). >>>> >>>> This doesn't seem to be a hard-and-fast policy; all of the VEC code I >>>> remember writing or looking at recently has no spaces, and I checked the >>>> patch in on that basis. However, running grep indicates that we have a >>>> profusion of styles... >>> >>> I think the style guide is quite clear on this: there should be a space >>> there as Jeff says. The fact that some code has crept in with other >>> styles doesn't make them right, or give justification for ignoring the >>> style guide. >> >> Would you like a patch for the hundreds of instances without spaces? >> >> Certainly vec.h never uses spaces; I thought this was simply The Way >> Things Were. >> >> -Nathan >> > > IMO, rototils are generally pointless. If you are fixing code nearby > then yes, fix the formatting issues. Otherwise, just don't exacerbate > the problem, or we'll reach the point where a rototil really does become > necessary. Well, the other approach is to have a commit hook which automatically deals with formatting crap by running indent, or whatever tool we want. That would take the decision out of the hands of the developer and free the reviewer from having to catch such things and ensures there is a canonical form for our sources. jeff -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJNmxQQAAoJEBRtltQi2kC7f7sH/RtTsEdYZKbgD1IdDTHnAW2R OW3ie04GuUdFC1ZAekvOcVhbIeouLZ/HyyaWiZZ3uNajF6cRDMIDe7MEygqKAWKg WUVtJeQlrhnERcvjJFe3pYzxf2wBwSYI/A+6Ql5mXfigx2Za7WoSdzq1zQXvd+Pe ihR35DClH2lX3YAqGi6h47J+lk0kRuN1kvnSWwOzo/ACYBkdJrbZDCRVtkV+UQnt zgn/NkUwNbnJmyApLlr5jVYRIbe8saVrjnrO39siOVz26eqnuV8IehY7ePnidr3f 8wZ06mTJaN/PgMpCltHvQZ3Vos/+BxTtCMTAFaxRKAJd25HNcd955GHBNG7qIYE= =UkkH -----END PGP SIGNATURE-----
On Tue, 2011-04-05 at 07:07 -0600, Jeff Law wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 04/05/11 06:55, Richard Earnshaw wrote: > > > > On Tue, 2011-04-05 at 05:30 -0700, Nathan Froyd wrote: > >> On Tue, Apr 05, 2011 at 01:22:39PM +0100, Richard Earnshaw wrote: > >>> On Tue, 2011-04-05 at 04:44 -0700, Nathan Froyd wrote: > >>>>> nit; You're missing some whitespace here (after the VEC). > >>>> > >>>> This doesn't seem to be a hard-and-fast policy; all of the VEC code I > >>>> remember writing or looking at recently has no spaces, and I checked the > >>>> patch in on that basis. However, running grep indicates that we have a > >>>> profusion of styles... > >>> > >>> I think the style guide is quite clear on this: there should be a space > >>> there as Jeff says. The fact that some code has crept in with other > >>> styles doesn't make them right, or give justification for ignoring the > >>> style guide. > >> > >> Would you like a patch for the hundreds of instances without spaces? > >> > >> Certainly vec.h never uses spaces; I thought this was simply The Way > >> Things Were. > >> > >> -Nathan > >> > > > > IMO, rototils are generally pointless. If you are fixing code nearby > > then yes, fix the formatting issues. Otherwise, just don't exacerbate > > the problem, or we'll reach the point where a rototil really does become > > necessary. > Well, the other approach is to have a commit hook which automatically > deals with formatting crap by running indent, or whatever tool we want. > > That would take the decision out of the hands of the developer and free > the reviewer from having to catch such things and ensures there is a > canonical form for our sources. Only if a tool either 1) Has a way to over-ride it when it's being stupid, or... 2) is never stupid... Most indent tools don't handle comments very well, IMO. For example: /* Some commentary text... Some Code Fragment some more commentary text. */ if you auto-indent that, you'll often end up with the code fragment, which was deliberately given extra indentation being moved to the same indentation as the surrounding text. In some cases that can lose important information. R.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 04/05/11 05:44, Nathan Froyd wrote: > On Mon, Apr 04, 2011 at 12:01:09PM -0600, Jeff Law wrote: >>> + canon_modify_mem_list = GCNEWVEC (VEC(modify_pair,heap) *, >>> + last_basic_block); >> nit; You're missing some whitespace here (after the VEC). > > This doesn't seem to be a hard-and-fast policy; all of the VEC code I > remember writing or looking at recently has no spaces, and I checked the > patch in on that basis. However, running grep indicates that we have a > profusion of styles... In a codebase as big as GCC, violations tends to slip in. If/when you see stuff, we'd love to see patches which fix such problems. jeff -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJNm0EOAAoJEBRtltQi2kC72/gH+gO3bpEldTzuEblKU5XGyhRw h92X43nPb7qZ/9SaH9RI+zc0XGRAclwqja7X360kFmC/b06AMTbakmsBkcIZDFUj MhXnt5jsOtQGAA4UlA1rPaUTA8IPN+QzIfIe8puN8Yf/W5Q0nxxKzJPbujXLkK0o +5dB8CXUOYQ0rAfkzleEUSdPi0iKm8wvg13HY83Q52EZvUQ3aWcxFPmXA0oIaz+v FQVhjjzr4YvHd2MaomofWC/t246bLVB+L6ofZnjh0jkhAikC0/z+FLG8en0VfGhr 0it6/hRIchlZfRV6jh/dN6vLR+NwP8FHwG25O/G+jnPRbut3fw3BaaQr4n3dnTQ= =vAx7 -----END PGP SIGNATURE-----
On Apr 5, 2011, at 5:55 AM, Richard Earnshaw wrote:
> IMO, rototils are generally pointless.
As a counter point, I like polish and style, in addition to beauty and flexibility. I'd rather see more style cleanups, more polish cleanups and more beautiful cleanups. I'd like to see more, not less people doing that work. I'd like to see more maintainership status give out on that basis. I like rototils. This is the mechanism by which we give a uniformity to the code, make it predictable, easier to work, easier to copy from.
On Tue, 5 Apr 2011, Nathan Froyd wrote: > On Tue, Apr 05, 2011 at 01:22:39PM +0100, Richard Earnshaw wrote: > > On Tue, 2011-04-05 at 04:44 -0700, Nathan Froyd wrote: > > > > nit; You're missing some whitespace here (after the VEC). > > > > > > This doesn't seem to be a hard-and-fast policy; all of the VEC code I > > > remember writing or looking at recently has no spaces, and I checked the > > > patch in on that basis. However, running grep indicates that we have a > > > profusion of styles... > > > > I think the style guide is quite clear on this: there should be a space > > there as Jeff says. The fact that some code has crept in with other > > styles doesn't make them right, or give justification for ignoring the > > style guide. > > Would you like a patch for the hundreds of instances without spaces? > > Certainly vec.h never uses spaces; I thought this was simply The Way > Things Were. I also had the impression that for certain special macros such as VEC, GTY, _, N_, G_ - macros that are not really substituting for functions, for-loops, etc. - the norm was no space, whereas for function prototypes, function calls and calls to macros that are being used syntactically and semantically more or less like functions the norm is to have the space (I'm not sure about the case of __attribute__ and macros generating __attribute__). But I see VEC is in fact used more often with the space. (For the VEC_* macros used like functions rather than in type names, the norm is very clearly to have the space.)
On Tue, Apr 05, 2011 at 07:18:16PM +0000, Joseph S. Myers wrote: > On Tue, 5 Apr 2011, Nathan Froyd wrote: > > Certainly vec.h never uses spaces; I thought this was simply The Way > > Things Were. > > I also had the impression that for certain special macros such as VEC, > GTY, _, N_, G_ - macros that are not really substituting for functions, > for-loops, etc. - the norm was no space, whereas for function prototypes, > function calls and calls to macros that are being used syntactically and > semantically more or less like functions the norm is to have the space > (I'm not sure about the case of __attribute__ and macros generating > __attribute__). But I see VEC is in fact used more often with the space. To be pedantic: grepping for 'VEC (' in gcc/ gives ~1750 hits. But a number of those are the X*VEC allocation functions and things like CLASSTYPE_METHOD_VEC; cutting those out ("fgrep 'VEC (' | egrep -v '[A-Z][A-Z_]+VEC ('") or similar gives a bit over 800 VEC instances that use spaces. grepping for 'VEC(' gives 1300+ hits. That's a lot of creeping laxity. :) -Nathan
diff --git a/gcc/gcse.c b/gcc/gcse.c index a1de61f..d6a4db4 100644 --- a/gcc/gcse.c +++ b/gcc/gcse.c @@ -385,8 +385,18 @@ static regset reg_set_bitmap; static rtx * modify_mem_list; static bitmap modify_mem_list_set; -/* This array parallels modify_mem_list, but is kept canonicalized. */ -static rtx * canon_modify_mem_list; +typedef struct modify_pair_s +{ + rtx dest; /* A MEM. */ + rtx dest_addr; /* The canonical address of `dest'. */ +} modify_pair; + +DEF_VEC_O(modify_pair); +DEF_VEC_ALLOC_O(modify_pair,heap); + +/* This array parallels modify_mem_list, except that it stores MEMs + being set and their canonicalized memory addresses. */ +static VEC(modify_pair,heap) **canon_modify_mem_list; /* Bitmap indexed by block numbers to record which blocks contain function calls. */ @@ -478,7 +488,6 @@ static void invalidate_any_buried_refs (rtx); static void compute_ld_motion_mems (void); static void trim_ld_motion_mems (void); static void update_ld_motion_stores (struct expr *); -static void free_insn_expr_list_list (rtx *); static void clear_modify_mem_tables (void); static void free_modify_mem_tables (void); static rtx gcse_emit_move_after (rtx, rtx, rtx); @@ -587,7 +596,8 @@ alloc_gcse_mem (void) /* Allocate array to keep a list of insns which modify memory in each basic block. */ modify_mem_list = GCNEWVEC (rtx, last_basic_block); - canon_modify_mem_list = GCNEWVEC (rtx, last_basic_block); + canon_modify_mem_list = GCNEWVEC (VEC(modify_pair,heap) *, + last_basic_block); modify_mem_list_set = BITMAP_ALLOC (NULL); blocks_with_calls = BITMAP_ALLOC (NULL); } @@ -1435,6 +1445,7 @@ canon_list_insert (rtx dest ATTRIBUTE_UNUSED, const_rtx unused1 ATTRIBUTE_UNUSED { rtx dest_addr, insn; int bb; + modify_pair *pair; while (GET_CODE (dest) == SUBREG || GET_CODE (dest) == ZERO_EXTRACT @@ -1453,10 +1464,9 @@ canon_list_insert (rtx dest ATTRIBUTE_UNUSED, const_rtx unused1 ATTRIBUTE_UNUSED insn = (rtx) v_insn; bb = BLOCK_FOR_INSN (insn)->index; - canon_modify_mem_list[bb] = - alloc_EXPR_LIST (VOIDmode, dest_addr, canon_modify_mem_list[bb]); - canon_modify_mem_list[bb] = - alloc_EXPR_LIST (VOIDmode, dest, canon_modify_mem_list[bb]); + pair = VEC_safe_push (modify_pair, heap, canon_modify_mem_list[bb], NULL); + pair->dest = dest; + pair->dest_addr = dest_addr; } /* Record memory modification information for INSN. We do not actually care @@ -1474,14 +1484,7 @@ record_last_mem_set_info (rtx insn) bitmap_set_bit (modify_mem_list_set, bb); if (CALL_P (insn)) - { - /* Note that traversals of this loop (other than for free-ing) - will break after encountering a CALL_INSN. So, there's no - need to insert a pair of items, as canon_list_insert does. */ - canon_modify_mem_list[bb] = - alloc_INSN_LIST (insn, canon_modify_mem_list[bb]); - bitmap_set_bit (blocks_with_calls, bb); - } + bitmap_set_bit (blocks_with_calls, bb); else note_stores (PATTERN (insn), canon_list_insert, (void*) insn); } @@ -1609,26 +1612,6 @@ compute_hash_table (struct hash_table_d *table) /* Expression tracking support. */ -/* Like free_INSN_LIST_list or free_EXPR_LIST_list, except that the node - types may be mixed. */ - -static void -free_insn_expr_list_list (rtx *listp) -{ - rtx list, next; - - for (list = *listp; list ; list = next) - { - next = XEXP (list, 1); - if (GET_CODE (list) == EXPR_LIST) - free_EXPR_LIST_node (list); - else - free_INSN_LIST_node (list); - } - - *listp = NULL; -} - /* Clear canon_modify_mem_list and modify_mem_list tables. */ static void clear_modify_mem_tables (void) @@ -1639,7 +1622,7 @@ clear_modify_mem_tables (void) EXECUTE_IF_SET_IN_BITMAP (modify_mem_list_set, 0, i, bi) { free_INSN_LIST_list (modify_mem_list + i); - free_insn_expr_list_list (canon_modify_mem_list + i); + VEC_free (modify_pair, heap, canon_modify_mem_list[i]); } bitmap_clear (modify_mem_list_set); bitmap_clear (blocks_with_calls); @@ -1710,25 +1693,19 @@ compute_transp (const_rtx x, int indx, sbitmap *bmap) blocks_with_calls, 0, bb_index, bi) { - rtx list_entry = canon_modify_mem_list[bb_index]; + VEC(modify_pair,heap) *list + = canon_modify_mem_list[bb_index]; + modify_pair *pair; + unsigned ix; - while (list_entry) + FOR_EACH_VEC_ELT_REVERSE (modify_pair, list, ix, pair) { - rtx dest, dest_addr; - - /* LIST_ENTRY must be an INSN of some kind that sets memory. - Examine each hunk of memory that is modified. */ - - dest = XEXP (list_entry, 0); - list_entry = XEXP (list_entry, 1); - dest_addr = XEXP (list_entry, 0); + rtx dest = pair->dest; + rtx dest_addr = pair->dest_addr; if (canon_true_dependence (dest, GET_MODE (dest), dest_addr, x, NULL_RTX, rtx_addr_varies_p)) - { - RESET_BIT (bmap[bb_index], indx); - } - list_entry = XEXP (list_entry, 1); + RESET_BIT (bmap[bb_index], indx); } } }
The patch below converts gcse.c:canon_modify_mem_list to hold VECs instead of EXPR_LIST rtxes. I am ambivalent about the use of VECs in canon_modify_mem_list; they will waste some memory compared to the linked list scheme present before, though I'm not sure how much. It would depend on the average chain length, etc. I'm happy to use an linked list datastructure instead, allocated out of an alloc_pool (better statistics!) or even gcse's private obstack if folks think that would be better. Moving things out of GC memory and eliminating a use of EXPR_LIST has to be considered a good thing, though... Doing this required addressing an odd little comment in record_last_mem_set_info: if (CALL_P (insn)) { /* Note that traversals of this loop (other than for free-ing) will break after encountering a CALL_INSN. So, there's no need to insert a pair of items, as canon_list_insert does. */ canon_modify_mem_list[bb] = alloc_INSN_LIST (insn, canon_modify_mem_list[bb]); bitmap_set_bit (blocks_with_calls, bb); } This is all well and good, except that the only real traversal of canon_modify_mem_list (compute_transp) doesn't check for CALL_INSNs. Instead, it has: EXECUTE_IF_AND_COMPL_IN_BITMAP (modify_mem_list_set, blocks_with_calls, 0, bb_index, bi) { rtx list_entry = canon_modify_mem_list[bb_index]; while (list_entry) { rtx dest, dest_addr; /* LIST_ENTRY must be an INSN of some kind that sets memory. Examine each hunk of memory that is modified. */ dest = XEXP (list_entry, 0); list_entry = XEXP (list_entry, 1); dest_addr = XEXP (list_entry, 0); Notice that since bits in blocks_with_calls are set if we find a CALL_INSN, we'll never examine canon_modify_mem_list for such blocks. Hence we can dispense with the spurious consing in record_last_mem_set_info. Tested on x86_64-unknown-linux-gnu. OK to commit? -Nathan * gcse.c (modify_pair): Define. Define a VEC of it. (canon_modify_mem_list): Convert to an array of VECs. (free_insn_expr_list_list): Delete. (clear_modify_mem_tables): Call VEC_free instead. (record_last_mem_set_info): Don't modify canon_modify_mem_list. (alloc_gcse_mem): Adjust for canon_modify_mem_list change. (canon_list_insert, compute_transp): Likewise. --- gcc/gcse.c | 79 +++++++++++++++++++++-------------------------------------- 1 files changed, 28 insertions(+), 51 deletions(-)