Re: volatile and R/M/W operations
On 1/12, Robert Dewar wrote: >> I cannot see a reason not to use "orb $32,y" here instead of a three >> steps read/modify/write operation. Is this only a missed optimization? >> (in which case I will open a PR) > > Are you sure it is an optimization, the timing on these things is > very subtle. What evidence do you have that there is a missed > optimization here? For this pattern (isolated setting of one bit in the middle of a byte at a random memory location), this is the best code on this platform AFAIK. As an evidence, if you mark neither variable as volatile GCC generates with -O3 -fomit-frame-pointer: f: orb $16, x orb $32, y ret And I sure expect that GCC didn't choose to generate worst code when I *removed* the volatile constraint :)
[RFC] Cleaning up latch blocks
Hello,
SMS currently works only on single-basic-block loops. This simplifies
the task of software pipelining. PR34263 is an example where outof-ssa
creates a non-empty latch block for a single-basic-block loop and thus
prevents SMS to be applied on it. This issue was raised in the past
(http://gcc.gnu.org/ml/gcc-patches/2005-11/msg01971.html) but I am not
sure what is the best approach to address it. Cleaning-up those latch
blocks for the propose of restoring the single-basic-block loop should
be helpful in general (and not only in the SMS perspective).
So, we can address this inside SMS or alternatively in outof-ssa as in the
attached patch (which was originally written by Andrew Pinski for PR19038
and rewritten by Mircea, it also passes bootstrap and regtest on ppc64).
Thanks,
Revital
(See attached file: patch_empty_latch_2_12.txt)Index: tree-outof-ssa.c
===
--- tree-outof-ssa.c(revision 130511)
+++ tree-outof-ssa.c(working copy)
@@ -871,6 +871,17 @@
BITMAP_FREE (leader_has_match);
}
+static tree
+contains_tree_r (tree *tp, int *walk_subtrees, void *data)
+{
+ if (*tp == data)
+{
+ *walk_subtrees = 0;
+ return data;
+}
+ else
+return NULL_TREE;
+}
/* Look at all the incoming edges to block BB, and decide where the best place
to insert the stmts on each edge are, and perform those insertions. */
@@ -944,7 +955,111 @@
if (count < 2)
{
if (single_edge)
-bsi_commit_one_edge_insert (single_edge, NULL);
+{
+ /* Loop back edges are special and should be handled that way. */
+ if (single_edge->dest == single_edge->src)
+{
+ bool do_it = true;
+ bool before = false;
+ tree stmts = PENDING_STMT (single_edge);
+ basic_block b_exit, b_pheader, b_loop = single_edge->src;
+ edge_iterator ei;
+ edge e;
+ block_stmt_iterator bsi_exit;
+
+ if (EDGE_COUNT (b_loop->succs) != 2
+ || EDGE_COUNT (b_loop->preds) != 2)
+do_it = false;
+
+ FOR_EACH_EDGE (e, ei, b_loop->succs)
+if (e->dest != b_loop)
+ break;
+
+ b_exit = e->dest;
+
+ if (EDGE_COUNT (b_exit->preds) != 1 || PENDING_STMT (e))
+do_it = false;
+
+ FOR_EACH_EDGE (e, ei, b_loop->preds)
+if (e->src != b_loop)
+ break;
+
+ b_pheader = e->src;
+
+ if (b_exit == b_pheader
+ || b_exit == b_loop || b_pheader == b_loop)
+do_it = false;
+
+ bsi_exit = bsi_after_labels (b_exit);
+ bsi = bsi_last (single_edge->src);
+ stmt = bsi_stmt (bsi);
+
+ /* Check if it is legal to do it. */
+ if (TREE_CODE (stmt) == COND_EXPR)
+{
+ tree expr = COND_EXPR_COND (stmt);
+ tree_stmt_iterator tsi;
+ before = true;
+
+ for (tsi = tsi_start (stmts); !tsi_end_p (tsi);
+ tsi_next (&tsi))
+{
+ tree stmt1 = tsi_stmt (tsi);
+ tree var;
+
+ if (TREE_CODE (stmt1) != GIMPLE_MODIFY_STMT)
+{
+ print_generic_stmt (dump_file, stmt1, TDF_VOPS);
+ do_it = false;
+ break;
+}
+ var = GIMPLE_STMT_OPERAND (stmt1, 0);
+ if (TREE_THIS_VOLATILE (var)
+ || TYPE_VOLATILE (TREE_TYPE (var))
+ || walk_tree (&expr, contains_tree_r, var, NULL))
+{
+ do_it = false;
+ break;
+}
+}
+}
+ /* Insert the statements right before */
+ if (do_it)
+{
+ tree_stmt_iterator tsi;
+
+ for (tsi = tsi_start (stmts); !tsi_end_p (tsi);
+ tsi_next (&tsi))
+{
+ tree stmt1 = tsi_stmt (tsi);
+ tree var, tmp_var, copy;
+
+ var = GIMPLE_STMT_OPERAND (stmt1, 0);
+ tmp_var = create_temp (var);
+ copy =
+build2 (GIMPLE_MODIFY_STMT, TREE_TYPE (tmp_var),
+tmp_var, var);
+ set_is_used (tmp_var);
+ if (before)
+bsi_insert_before (&bsi, copy, BSI_SAME_STMT);
+ else
+bsi_insert_after (&bsi, copy, BSI_NEW_STMT);
+ copy =
+build2 (GIMPLE_MOD
Rant about ChangeLog entries and commit messages
As a recent committer to GCC, I am always surprised to see the content of ChangeLog entries and commit messages. I tend to find GCC ChangeLog entries useless. For example, the more recent ChangeLog entry in gcc/ChangeLog is: | 2007-11-30 Jan Hubicka <[EMAIL PROTECTED]> | | * ggc-common.c (dump_ggc_loc_statistics): Reset ggc_force_collect | flag. How could a newcomer guess why the gcc_force_collect flag needs to be reset? Jan posted a useful explanation on gcc-patches, but finding it by searching the mailing-list is not practical and it is not coupled with the checkin. Let's look at the corresponding svn log message, which can be found with "svn blame" if a particular line needs to be pinpointed: | r130560 | hubicka | 2007-12-01 22:06:31 +0100 (Sat, 01 Dec 2007) | 4 lines | | * ggc-common.c (dump_ggc_loc_statistics): Reset ggc_force_collect | flag. Ok, same information is mirrored here, not useful. Let's look at the change itself then: | Index: gcc/ChangeLog | === | --- gcc/ChangeLog (revision 130559) | +++ gcc/ChangeLog (revision 130560) | @@ -1,3 +1,8 @@ | +2007-11-30 Jan Hubicka <[EMAIL PROTECTED]> | + | + * ggc-common.c (dump_ggc_loc_statistics): Reset ggc_force_collect | + flag. | + | 2007-11-30 Seongbae Park <[EMAIL PROTECTED]> | | PR rtl-optimization/34171 | Index: gcc/ggc-common.c | === | --- gcc/ggc-common.c(revision 130559) | +++ gcc/ggc-common.c(revision 130560) | @@ -1018,5 +1018,6 @@ |fprintf (stderr, "%-48s %10s %10s %10s %10s %10s\n", |"source location", "Garbage", "Freed", "Leak", "Overhead", "Times"); |fprintf (stderr, "---\n") | ; | + ggc_force_collect = false; | #endif | } Ok, still the same information because of the ChangeLog diff, and we can see that the change shows that... gcc_force_collect is reset. Wow! Now, compare that with Jan's message on the list: | pre-ipa-mem-reports force GGC to be done at each invokation in order to | collect data on live memory references, but forgets to reset the flag | after done. This means that compiler continues GGCcollecting and works | slowly. This *is* the information I would expect to be present somewhere in GCC history. A clear and detailed information on why the change was necessary. Sure, in some case the checkin references a PR, but the PR often contains information of what didn't work before the change and the same information which is already repeated three times (ChangeLog, svn log and svn diff). Compare this to a typical commit in the Linux kernel: | commit b1812582ba94b5f377d5d3cec7646cc17d84e733 | Author: Joachim Fenkes <[EMAIL PROTECTED]> | Date: Fri Nov 30 16:19:41 2007 -0800 | | IB/ehca: Fix static rate if path faster than link | | The formula would yield -1 if the path is faster than the link, which | is wrong in a bad way (max throttling). Clamp to 0, which is the | correct value. | | Signed-off-by: Joachim Fenkes <[EMAIL PROTECTED]> | Signed-off-by: Roland Dreier <[EMAIL PROTECTED]> | | diff --git a/drivers/infiniband/hw/ehca/ehca_av.c b/drivers/infiniband/hw/ehca/ehca_av.c | index 453eb99..f7782c8 100644 | --- a/drivers/infiniband/hw/ehca/ehca_av.c | +++ b/drivers/infiniband/hw/ehca/ehca_av.c | @@ -76,8 +76,12 @@ int ehca_calc_ipd(struct ehca_shca *shca, int port, | | link = ib_width_enum_to_int(pa.active_width) * pa.active_speed; | | - /* IPD = round((link / path) - 1) */ | - *ipd = ((link + (path >> 1)) / path) - 1; | + if (path >= link) | + /* no need to throttle if path faster than link */ | + *ipd = 0; | + else | + /* IPD = round((link / path) - 1) */ | + *ipd = ((link + (path >> 1)) / path) - 1; | | return 0; | } What do we have here? A one-line high-level description identifying what has been done, a synthetic analysis of the problem cause and its solution, and the audit trail with the Signed-off-by lines (which in the Linux case is more important than in GCC as copyrights are not assigned to one entity). Linux doesn't use ChangeLog, but its history is much more useful to developers and casual observers than GCC one. And it could be done for GCC (with SVN) as well. Maybe we should consider dropping ChangeLogs and using better checkin messages. Sam
Re: Rant about ChangeLog entries and commit messages
Samuel Tardieu <[EMAIL PROTECTED]> writes: > As a recent committer to GCC, I am always surprised to see the content > of ChangeLog entries and commit messages. > > I tend to find GCC ChangeLog entries useless. For example, the more > recent ChangeLog entry in gcc/ChangeLog is: > > | 2007-11-30 Jan Hubicka <[EMAIL PROTECTED]> > | > | * ggc-common.c (dump_ggc_loc_statistics): Reset ggc_force_collect > | flag. > > How could a newcomer guess why the gcc_force_collect flag needs to be > reset? That is supposed to be written in a comment. The change log entry should describe _what_ is being changed, so that you can find out when a particular change was made. Andreas. -- Andreas Schwab, SuSE Labs, [EMAIL PROTECTED] SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Re: Rant about ChangeLog entries and commit messages
> I tend to find GCC ChangeLog entries useless. For example, the more > > recent ChangeLog entry in gcc/ChangeLog is: > | 2007-11-30 Jan Hubicka <[EMAIL PROTECTED]> > | > | * ggc-common.c (dump_ggc_loc_statistics): Reset ggc_force_collect > | flag. > > How could a newcomer guess why the gcc_force_collect flag needs to be > reset? He indeed cannot, but the ChangeLog is not meant to make it possible either. See http://gcc.gnu.org/contribute.html, especially the GNU Coding Standards. > Jan posted a useful explanation on gcc-patches, but finding it > by searching the mailing-list is not practical and it is not coupled > with the checkin. That's how it has always worked so it should be more or less practical. For PRs, there is a link (URL: field), maybe we should use PRs more often. -- Eric Botcazou
Re: Rant about ChangeLog entries and commit messages
On 2/12, Eric Botcazou wrote: | He indeed cannot, but the ChangeLog is not meant to make it possible either. | See http://gcc.gnu.org/contribute.html, especially the GNU Coding Standards. I know this document and I think the part on ChangeLog doesn't achieve its purpose: http://www.gnu.org/prep/standards/standards.html#Change-Logs Keep a change log to describe all the changes made to program source files. The purpose of this is so that people investigating bugs in the future will know about the changes that might have introduced the bug. Often a new bug can be found by looking at what was recently changed. More importantly, change logs can help you eliminate conceptual inconsistencies between different parts of a program, by giving you a history of how the conflicting concepts arose and who they came from. This is precisely why I am proposing an evolution in the current process. Also, this document states: There's no need to describe the full purpose of the changes or how they work together. If you think that a change calls for explanation, you're probably right. Please do explain it—but please put the explanation in comments in the code, where people will see it whenever they see the code. When you fix a bug by changing a constant (for example if there has been an offset by one error or, as I did a few minutes ago in config/sh/sh.md, there was an error in the argument to consider), this doesn't always mandate a comment in the code. For example, I think a description such as the one I wrote when describing the problem cmpgeusi_t splitting code compares operand 0 to 0, while this constant value can only be in operand 1. When compiling the Ada runtime, this leads to a "cmp/hs #0,r7" instruction which is not valid as "cmp/hs" operands must be two registers. along with the above change would have been a better commit message than just gcc/ * config/sh/sh.md (cmpgeusi_t): Fix condition. which I used as suggested. | That's how it has always worked so it should be more or less practical. Sure, it works. But this is not a reason not to improve the process. | For PRs, there is a link (URL: field), maybe we should use PRs more often. This field is useful to look at the discussion that led to the change, but PRs often contain no synthetic information on the analysis of the problem unless when the PR submitter sends a patch himself (in which case he often includes his analysis to get a better chance to get his patch checked in).
Re: Rant about ChangeLog entries and commit messages
> I know this document and I think the part on ChangeLog doesn't achieve > its purpose: > > http://www.gnu.org/prep/standards/standards.html#Change-Logs > >Keep a change log to describe all the changes made to program source >files. The purpose of this is so that people investigating bugs in the >future will know about the changes that might have introduced the bug. >Often a new bug can be found by looking at what was recently changed. >More importantly, change logs can help you eliminate conceptual >inconsistencies between different parts of a program, by giving you a >history of how the conflicting concepts arose and who they came from. Could you elaborate? > When you fix a bug by changing a constant (for example if there has been > an offset by one error or, as I did a few minutes ago in > config/sh/sh.md, there was an error in the argument to consider), this > doesn't always mandate a comment in the code. For example, I think a > description such as the one I wrote when describing the problem > >cmpgeusi_t splitting code compares operand 0 to 0, while this constant >value can only be in operand 1. When compiling the Ada runtime, this >leads to a "cmp/hs #0,r7" instruction which is not valid as "cmp/hs" >operands must be two registers. > > along with the above change would have been a better commit message than > just > > gcc/ > * config/sh/sh.md (cmpgeusi_t): Fix condition. > > which I used as suggested. Not really in my opinion, it's a trivial fix and totally unrelated to Ada in itself, "Fix typo" or "Fix obvious mistake" would have been just fine too. -- Eric Botcazou
Re: Rant about ChangeLog entries and commit messages
On 2/12, Andreas Schwab wrote: | That is supposed to be written in a comment. The change log entry | should describe _what_ is being changed, so that you can find out when a | particular change was made. This should be the job of the VCS, e.g. "svn log" and "svn blame". Moreover, ChangeLogs are organized by directories. You have to look at the "svn log" to see if a test corresponds to a code change and identify it.
Re: Rant about ChangeLog entries and commit messages
On 2/12, Eric Botcazou wrote: | > I know this document and I think the part on ChangeLog doesn't achieve | > its purpose: | > | > http://www.gnu.org/prep/standards/standards.html#Change-Logs | > | >Keep a change log to describe all the changes made to program source | >files. The purpose of this is so that people investigating bugs in the | >future will know about the changes that might have introduced the bug. | >Often a new bug can be found by looking at what was recently changed. | >More importantly, change logs can help you eliminate conceptual | >inconsistencies between different parts of a program, by giving you a | >history of how the conflicting concepts arose and who they came from. | | Could you elaborate? I'll take an example from one of your recent changes in gcc/ChangeLog: 2007-11-19 Eric Botcazou <[EMAIL PROTECTED]> * stor-layout.c (lang_adjust_rli): Delete. (set_lang_adjust_rli): Likewise. (layout_type): Do not call lang_adjust_rli hook. * tree.h (set_lang_adjust_rli): Delete. Without digging in the mailing-list archives to see why you made the change, if something new breaks on a STABS platform I will have no hint that this change was in any way related to STABS. If we didn't use ChangeLogs and if commit logs contained the information you gave on the mailing-list, this would be much easier: The compiler has been broken on STABS platform since mapped locations were enabled by default. The Ada front-end is emitting debug info too early. The patch also reorganizes a little the front-end's initialization and gets rid of dead code in the process, which in turn enables a further cleanup in the middle-end. Also note that the ChangeLog doesn't give any hint that changes in the ada directory have been made at the same time, only "svn log" reveals that. So far for the "The purpose of this is so that people investigating bugs in the future will know about the changes that might have introduced the bug." sentence. I would prefer that information which is deemed necessary for peer-review when a patch is sent to gcc-patches@ is also included in GCC log history. | > [sh.md fix] | | Not really in my opinion, it's a trivial fix and totally unrelated to Ada in | itself, "Fix typo" or "Fix obvious mistake" would have been just fine too. Well, I find it useful to know which part of the compiler has exercized this code path (as obviously there was no test associated with this optimization) and uncovered the bug, but I agree that this was an obvious typo fix.
Re: Rant about ChangeLog entries and commit messages
> I'll take an example from one of your recent changes in gcc/ChangeLog: > >2007-11-19 Eric Botcazou <[EMAIL PROTECTED]> > >* stor-layout.c (lang_adjust_rli): Delete. >(set_lang_adjust_rli): Likewise. >(layout_type): Do not call lang_adjust_rli hook. >* tree.h (set_lang_adjust_rli): Delete. > > Without digging in the mailing-list archives to see why you made the > change, if something new breaks on a STABS platform I will have no hint > that this change was in any way related to STABS. But this change has nothing to do with STABS. :-) > Also note that the ChangeLog doesn't give any hint that changes in the > ada directory have been made at the same time, only "svn log" reveals > that. So far for the "The purpose of this is so that people investigating > bugs in the future will know about the changes that might have introduced > the bug." sentence. Well, any changes in the compiler can potentially introduce bugs elsewhere and I suppose that you aren't proposing to mention all the known dependencies in the commit message. -- Eric Botcazou
Re: Rant about ChangeLog entries and commit messages
Sam> How could a newcomer guess why the gcc_force_collect flag needs to Sam> be reset? Andreas> That is supposed to be written in a comment. The change log Andreas> entry should describe _what_ is being changed, so that you Andreas> can find out when a particular change was made. Out of curiosity, I looked for an example with one of your own commits in the Linux kernel tree and I found one: I think http://tinyurl.com/2j7lt7 is a very helpful explanation of the corresponding change (http://tinyurl.com/2tpw8l). Someone trying to fix a similar bug in another driver will benefit from having this message in the VCS history. A comment in the code would probably have been much shorter than this explanation and would probably not contain the "headphone", "line out" and "muted" words. Once again, I agree that the current mechanism works for GCC developers, but I think it could be much better if: 1- commit messages didn't duplicate ChangeLog entries (maybe by getting rid of ChangeLogs) 2- commit messages contained a synthetic information such as the one provided for peer-review I'm not trying to launch a revolution in the GCC development process, I'm only comparing two ways of documenting changes as they are committed and explaining why I find the Linux way of doing it more useful. As a side note, I know several (sick?) people (including myself) who casually read the Linux kernel RSS feed in their RSS aggregator and find it very insightful (if you exclude the "Merge" messages) while lighter than reading the whole linux-kernel mailing-list. People reading this can look at http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=atom with RSS-capable software to see what I mean. The GCC ChangeLogs, even when aggregated, aren't as nice to read when you're having breakfast :) Sam
Re: Rant about ChangeLog entries and commit messages
> "Eric" == Eric Botcazou <[EMAIL PROTECTED]> writes: >> Without digging in the mailing-list archives to see why you made >> the change, if something new breaks on a STABS platform I will have >> no hint that this change was in any way related to STABS. Eric> But this change has nothing to do with STABS. :-) Sure, but as you explained yourself in the message I cited, the reason to do this change was because of a problem in STABS info generation :) Eric> Well, any changes in the compiler can potentially introduce bugs Eric> elsewhere and I suppose that you aren't proposing to mention all Eric> the known dependencies in the commit message. Of course not, but when such a dependency is the reason you make a change in the first place, I think it ought to be mentionned. If the chance caused a regression on an obscure platform using some barely used debugging format, this could give a hint of where to look (STABS for example) to see how it is done there and if bogus assumptions were made about the previous behaviour. Sam -- Samuel Tardieu -- [EMAIL PROTECTED] -- http://www.rfc1149.net/
Re: Rant about ChangeLog entries and commit messages
On Sun, 2 Dec 2007, Samuel Tardieu wrote: > On 2/12, Andreas Schwab wrote: > > | That is supposed to be written in a comment. The change log entry > | should describe _what_ is being changed, so that you can find out when a > | particular change was made. > > This should be the job of the VCS, e.g. "svn log" and "svn blame". > Moreover, ChangeLogs are organized by directories. You have to look > at the "svn log" to see if a test corresponds to a code change and > identify it. The comment *in the code* is lacking, other than that I don't see much point in your rant; it's all been said before, for one. It usually takes a while for newcomers to understand the process, in particular the what-not-why of ChangeLogs... ;) brgds, H-P
Re: Rant about ChangeLog entries and commit messages
> Sure, but as you explained yourself in the message I cited, the reason > to do this change was because of a problem in STABS info generation :) "reason" is not quite appropriate in this case, "occasion" is more accurate. -- Eric Botcazou
Re: Rant about ChangeLog entries and commit messages
> > How could a newcomer guess why the gcc_force_collect flag needs to be > > reset? > > That is supposed to be written in a comment. The change log entry > should describe _what_ is being changed, so that you can find out when a > particular change was made. Not quite. The comments are supposed to say why the code is doing what it's doing (and, where it's helpful, why it ISN'T doing something else). Purely historical references in the comments that don't serve to clarify the present code are discouraged. (We don't want comments turning in a blog, for example.) I view the description in the gcc-patches message as PART of the CM history of GCC in that IT'S the place to go to get this information. What's unfortunate, I think, is that there's no easy way to find this message from the CM revision number.
Re: Rant about ChangeLog entries and commit messages
Richard Kenner wrote: >>> How could a newcomer guess why the gcc_force_collect flag needs to be >>> reset? >> That is supposed to be written in a comment. The change log entry >> should describe _what_ is being changed, so that you can find out when a >> particular change was made. > > Not quite. The comments are supposed to say why the code is doing what > it's doing (and, where it's helpful, why it ISN'T doing something else). > Purely historical references in the comments that don't serve to clarify > the present code are discouraged. (We don't want comments turning in a > blog, for example.) > > I view the description in the gcc-patches message as PART of the CM history > of GCC in that IT'S the place to go to get this information. What's > unfortunate, I think, is that there's no easy way to find this message from > the CM revision number. I think that's Samuel's point - it would be much better to have them in the commit log. FWIW, I agree completely - I've never found ChangeLogs useful, I hate writing them, and I think the linux-kernel guys these days generally have much better checkin messages than we do. Bernd -- This footer brought to you by insane German lawmakers. Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif
Re: Rant about ChangeLog entries and commit messages
> FWIW, I agree completely - I've never found ChangeLogs useful, I hate > writing them, and I think the linux-kernel guys these days generally have > much better checkin messages than we do. I guess nobody really loves writing ChangeLog entries, but in my opinion there are quite effective "executive summaries" for the patches and helpful to the reader/reviewer. Please let's not throw the baby with the bath's water. -- Eric Botcazou
Re: Rant about ChangeLog entries and commit messages
[ Charset ISO-8859-1 unsupported, converting... ] > > FWIW, I agree completely - I've never found ChangeLogs useful, I hate > > writing them, and I think the linux-kernel guys these days generally have > > much better checkin messages than we do. > > I guess nobody really loves writing ChangeLog entries, but in my opinion > there > are quite effective "executive summaries" for the patches and helpful to the > reader/reviewer. Please let's not throw the baby with the bath's water. If there's a mechanism to filter checkin messages to ChangeLog summaries, I would be happy to use it - in cases of multiple packages, especially, it's important to know what changes were made, when, and when the changes propagated through packages and releases, and where they got to, occasionally. Anybody know of a useful, built-in mechanism for this task? -- Ctalk Home Page: http://ctalk-lang.sourceforge.net
Re: Rant about ChangeLog entries and commit messages
Samuel Tardieu <[EMAIL PROTECTED]> writes: > recent ChangeLog entry in gcc/ChangeLog is: >From my understanding the gcc changelogs serve two purposes these days: - Force the submitter to read (or rather speed read) the patch again before sending it out. - Serve as a "hash key" to search the gcc-patches archives for the real changelog. The second could be solved far better in many obvious ways, but it's unclear how to replace the first. Linux kernel doesn't have a solution for it. -Andi
Re: Rant about ChangeLog entries and commit messages
On 12/2/07, Richard Kenner <[EMAIL PROTECTED]> wrote: > > > How could a newcomer guess why the gcc_force_collect flag needs to be > > > reset? > > > > That is supposed to be written in a comment. The change log entry > > should describe _what_ is being changed, so that you can find out when a > > particular change was made. > > Not quite. The comments are supposed to say why the code is doing what > it's doing (and, where it's helpful, why it ISN'T doing something else). > Purely historical references in the comments that don't serve to clarify > the present code are discouraged. (We don't want comments turning in a > blog, for example.) > > I view the description in the gcc-patches message as PART of the CM history > of GCC in that IT'S the place to go to get this information. What's > unfortunate, I think, is that there's no easy way to find this message from > the CM revision number. > Nothing stops people from putting URL's. However, I'd much rather see us put more detailed explanations in svn log or the ChangeLog than try to associate mailing list threads with commits. I'm certainly not going to hunt down the URL for every thread for every patch I commit.
Re: Rant about ChangeLog entries and commit messages
On 12/2/07, Bernd Schmidt <[EMAIL PROTECTED]> wrote: > Richard Kenner wrote: > >>> How could a newcomer guess why the gcc_force_collect flag needs to be > >>> reset? > >> That is supposed to be written in a comment. The change log entry > >> should describe _what_ is being changed, so that you can find out when a > >> particular change was made. > > > > Not quite. The comments are supposed to say why the code is doing what > > it's doing (and, where it's helpful, why it ISN'T doing something else). > > Purely historical references in the comments that don't serve to clarify > > the present code are discouraged. (We don't want comments turning in a > > blog, for example.) > > > > I view the description in the gcc-patches message as PART of the CM history > > of GCC in that IT'S the place to go to get this information. What's > > unfortunate, I think, is that there's no easy way to find this message from > > the CM revision number. > > I think that's Samuel's point - it would be much better to have them in > the commit log. FWIW, I agree completely - I've never found ChangeLogs > useful, I hate writing them, and I think the linux-kernel guys these > days generally have much better checkin messages than we do. > +1. I have never, in 7 years of working on and debugging gcc, found the ChangeLog to be useful in debugging a problem. They are like a useless version of svn diff output. If I wanted to know what the change did, i'd look at what the change did. The ChangeLog is well nigh useless even for that compared to the actual diff. I'd go even further, and say if the GNU coding standards say we shouldn't be putting descriptions of why we are changing things in the ChangeLog, than they should be changed and should be ignored on this point until they do. Pointing to them as the if they are The One True Way seems very suspect to me. After all, how else would they ever improve if nobody tries anything different? --Dan
Re: Rant about ChangeLog entries and commit messages
On 12/2/07, Eric Botcazou <[EMAIL PROTECTED]> wrote: > > I'd go even further, and say if the GNU coding standards say we > > shouldn't be putting descriptions of why we are changing things in the > > ChangeLog, than they should be changed and should be ignored on this > > point until they do. Pointing to them as the if they are The One True > > Way seems very suspect to me. After all, how else would they ever > > improve if nobody tries anything different? > > The people who wrote them presumably thought about these issues, too. Right, because surely one size fits all projects and possibilities, and workflow and processes have certainly not changed since then.
Re: Rant about ChangeLog entries and commit messages
> I'd go even further, and say if the GNU coding standards say we > shouldn't be putting descriptions of why we are changing things in the > ChangeLog, than they should be changed and should be ignored on this > point until they do. Pointing to them as the if they are The One True > Way seems very suspect to me. After all, how else would they ever > improve if nobody tries anything different? The people who wrote them presumably thought about these issues, too. -- Eric Botcazou
Re: Rant about ChangeLog entries and commit messages
On Sun, 2007-12-02 at 21:36 +0100, Eric Botcazou wrote: > > I'd go even further, and say if the GNU coding standards say we > > shouldn't be putting descriptions of why we are changing things in the > > ChangeLog, than they should be changed and should be ignored on this > > point until they do. Pointing to them as the if they are The One True > > Way seems very suspect to me. After all, how else would they ever > > improve if nobody tries anything different? > > The people who wrote them presumably thought about these issues, too. > Unfortunately they didn't document the "why" just the "what"! Tim Josling
Re: Rant about ChangeLog entries and commit messages
On Sun, 2007-12-02 at 09:26 -0500, Robert Kiesling wrote: > > I guess nobody really loves writing ChangeLog entries, but in my opinion > > there > > are quite effective "executive summaries" for the patches and helpful to > > the > > reader/reviewer. Please let's not throw the baby with the bath's water. > > If there's a mechanism to filter checkin messages to ChangeLog summaries, > I would be happy to use it - in cases of multiple packages, especially, it's > important to know what changes were made, when, and when the changes > propagated > through packages and releases, and where they got to, occasionally. Anybody > know of a useful, built-in mechanism for this task? > Personally I find it slow and inefficient tracing through why a given change was made. It is just a slow process searching and sometimes I don't bother because it is so inconvenient. The ChangeLog entries provide little help and there does not seem to be a good alternative. If there is a good alternative no-one has said what it is so far. As people have pointed out, the RCSs pretty well cover the "what" these days. And writing changelog entries, which largely duplicate this information, is time-consuming and tedious. And there are of of little to no value to me at least. The coding standards do allow, in some cases, that giving some context would be useful: > See also what the GNU Coding Standards have to say about what goes in > ChangeLogs; in particular, descriptions of the purpose of code and > changes should go in comments rather than the ChangeLog, though a > single line overall description of the changes may be useful above the > ChangeLog entry for a large batch of changes. I personally would strongly favour each ChangeLog entry having a single line of context. This could be the PR number or a single line giving the purpose of the change or what bigger change it is part of. As pointed out by Zach Weinberg in his paper "A Maintenance Programmer's View of GCC", there are many impediments to contributing to GCC. http://www.linux.org.uk/~ajh/gcc/gccsummit-2003-proceedings.pdf Things are not much better than they were when Zach wrote his paper. This small change would be one positive step n the right direction, IMHO. Tim Josling
Re: Link tests after GCC_NO_EXECUTABLES
Richard Sandiford wrote: > Anyway, given that there have been objections to the patch generally, > I realise that the pre-approval is void. I think there's no controversy over the libstdc++ change, so let's put that in. If nothing else, it makes the libstdc++ configury more self-consistent; if we decide to change the overall strategy, then we can do that all at once. Thanks, -- Mark Mitchell CodeSourcery [EMAIL PROTECTED] (650) 331-3385 x713
Re: Link tests after GCC_NO_EXECUTABLES
Rask Ingemann Lambertsen wrote:
>So, here is the patch to implement the config.cache file trick: Create a
> config.cache file with all the right link test answers for newlib just
> before running configure, in both Makefile.tpl and config-ml.in. This allows
> sparc-unknown-elf to build libstdc++-v3 with unmodified
> libstdc++-v3/configure.ac. Libgfortran's configure.ac needs just the symbol
> versioning patch ported from libssp. And that's it!
This trick seems plausible to me. Certainly, if it works, it would
simplify development of configure scripts for run-time libraries.
My only concern with this approach is that Newlib might not be entirely
consistent across configurations and architectures. The libstdc++
approach presumably entails some manual verification of each function's
presence or absence; before we claim that "Newlib has foo" someone
verifies that. I don't know if this is a problem in practice.
For example, these lines seem like things that might vary.
> +have_fpsetmask=${have_fpsetmask=no}
...
> +have_sync_fetch_and_add=${have_sync_fetch_and_add=no}
I suppose we could solve that problem, if it arises, with different
config.cache files for different targets. Perhaps it would be best to
generalize this by adding a top-level --with-target-lib-cache= option,
and then, if that's not present, and $with_newlib is set, passing in the
Newlib cache that you have?
That would give people a way to say that for their particular RTOS
and/or C library the following functions are available.
In theory, at least, we might also have differences between multilibs.
It Would Be Nice to be moving GCC in the direction of allowing different
multilibs for different operating systems and/or C libraries. So, I
suppose the all-singing, all-dancing version of this would be some
option that allows you to specify a cache file per multilib. But, I
think that could be left for later.
What do you and others think?
--
Mark Mitchell
CodeSourcery
[EMAIL PROTECTED]
(650) 331-3385 x713
Re: Rant about ChangeLog entries and commit messages
> Right, because surely one size fits all projects and possibilities, It was supposed to be the Coding Standards for The GNU Project. > and workflow and processes have certainly not changed since then. In my opinion, it's now easier to work around their perceived deficiencies. -- Eric Botcazou
Re: Rant about ChangeLog entries and commit messages
> This *is* the information I would expect to be present somewhere in > GCC history. A clear and detailed information on why the change was > necessary. Sure, in some case the checkin references a PR, but the PR > often contains information of what didn't work before the change and > the same information which is already repeated three times (ChangeLog, > svn log and svn diff). Keep in mind that the GNU coding standard introduced ChangeLogs before networked version control systems. In those days, you would receive a GCC release tarball with a ChangeLog. There was no way to do "svn log" or "svn diff" operations. Even in recent years, I have worked on GCC trees that were exported from the version control systems of other companies and that I did not have access to. In these situations, ChangeLogs are quite a bit more valuable. Having said that, I find the lack of rationale for some changes to be a bit irritating. I know that this should be done through code comments, but those are often made across the changeset and in different files. There is rarely a single summary of the need for the change. It would be nice to consider a practice similar to that used by NetBSD, which is to use a paragraph or so describing the need for the change (similar to what we do when we introduce a patch on gcc-patches) and inserting that comment into the svn commit message. Ben
Re: Describing commercial support on our website
> But the problem with ordering based on contributions is that people > will then fight over whether company A or company B has contributed > more; also, people who do their homework will know about, say, > CodeSourcery's role in GCC even if we sort the list by alphabetical > order. I'd rather avoid those kind of judgment calls. Indeed. Why do you think so many plumbers call their businesses AAA Plumbing? :-) Ben
Re: Rant about ChangeLog entries and commit messages
Hans-Peter Nilsson wrote: The comment *in the code* is lacking, other than that I don't see much point in your rant; it's all been said before, for one. It usually takes a while for newcomers to understand the process, in particular the what-not-why of ChangeLogs... ;) I actually think that often it is helpful to have the why in changelogs. Yes, this should never take the place of commments in the code, and that is a failing you want to watch out for, but sometimes a global motivation for a chage would make a changelog entry easier to understand. If all the changelog entry says is something like (xyz): new function I don't see much point, since a diff can always easily tell you *what* was changed. brgds, H-P
Re: Rant about ChangeLog entries and commit messages
Eric Botcazou wrote: FWIW, I agree completely - I've never found ChangeLogs useful, I hate writing them, and I think the linux-kernel guys these days generally have much better checkin messages than we do. I guess nobody really loves writing ChangeLog entries, but in my opinion there are quite effective "executive summaries" for the patches and helpful to the reader/reviewer. Please let's not throw the baby with the bath's water. Right, but to me an important part of the "executive summary" is why you did it. You don't go to the boss and say "I fired Joe", you go and say "I fired Joe, because ..."
Re: Rant about ChangeLog entries and commit messages
On Sun, 2 Dec 2007, Daniel Berlin wrote: > I have never, in 7 years of working on and debugging gcc, found the > ChangeLog to be useful in debugging a problem. I find they are useful for finding what has changed in function X (or in functions matching pattern Y) since 4.1, say (given a bug in 4.1-based sources that might be fixed by a backport of a more recent patch, which has been traced to involve function X in some way). The key feature here of course is not that the logs do not contain "why", but that they do contain the names of all the functions changed (beyond purely mechanical "all callers changed" type changes) - and the function names can be stable even as the functions themselves move between source files. I think that part of the standards remains useful with logs with the more detailed "why" as used in the gcc/ada/ directory. -- Joseph S. Myers [EMAIL PROTECTED]
Re: Rant about ChangeLog entries and commit messages
Eric Botcazou wrote: I'd go even further, and say if the GNU coding standards say we shouldn't be putting descriptions of why we are changing things in the ChangeLog, than they should be changed and should be ignored on this point until they do. Pointing to them as the if they are The One True Way seems very suspect to me. After all, how else would they ever improve if nobody tries anything different? The people who wrote them presumably thought about these issues, too. Maybe so, but I guess we only have a record of what they came up with and not why :-) :-) In the Ada revision histories, we have always given the what-and-the-why (and if necessary the why-not), and they have proved very helpful, I always found the RH's for gigi (done in the gcc style, much less helpful because they omitted the why). Of course you have to watch out for people forgetting that RH's never substitute for comments, but all patches are reviewed, and if that happens it gets fixed during the review process.
Re: Rant about ChangeLog entries and commit messages
On Mon, Dec 03, 2007 at 10:02:13AM +1100, Ben Elliston wrote: > Having said that, I find the lack of rationale for some changes to be a > bit irritating. I know that this should be done through code comments, > but those are often made across the changeset and in different files. > There is rarely a single summary of the need for the change. It would > be nice to consider a practice similar to that used by NetBSD, which is > to use a paragraph or so describing the need for the change (similar to > what we do when we introduce a patch on gcc-patches) and inserting that > comment into the svn commit message. Or even into the ChangeLog... I've worked on other projects that did this. I found it incredibly helpful. -- Daniel Jacobowitz CodeSourcery
Re: volatile and R/M/W operations
Samuel Tardieu wrote: For this pattern (isolated setting of one bit in the middle of a byte at a random memory location), this is the best code on this platform AFAIK. As an evidence, if you mark neither variable as volatile GCC generates with -O3 -fomit-frame-pointer: f: orb $16, x orb $32, y ret And I sure expect that GCC didn't choose to generate worst code when I *removed* the volatile constraint :) OK, sounds reasonable, but then I don't understand the logic behind avoiding this instruction sequence for the volatile case, this is two accesses at the bus level so what's the difference? I think on earlier pentiums these instructions were supposed to be avoided but of course this may have changed.
Re: Rant about ChangeLog entries and commit messages
On 12/2/07, Eric Botcazou <[EMAIL PROTECTED]> wrote: > > Right, because surely one size fits all projects and possibilities, > > It was supposed to be the Coding Standards for The GNU Project. Sorry, but again, this is not a good enough justification to me. We do a lot of things different than "The GNU Project". So do plenty of parts of the "official GNU project". They use different coding standards, bug tracking systems, version control systems, checkin policies, etc, than each other. If you have a better justification, i'd love to hear it.
Re: Rant about ChangeLog entries and commit messages
Robert Dewar <[EMAIL PROTECTED]> writes: > I don't see much point, since a diff can always easily tell > you *what* was changed. A changelog does help recreate a change *set* though, since CVS is lacking such a thing. Thus, the CL helps you determine what files to diff. True that SVN solves part of that, but SVN is not universal.
Re: Rant about ChangeLog entries and commit messages
> On Sun, 2007-12-02 at 09:26 -0500, Robert Kiesling wrote: > > > I guess nobody really loves writing ChangeLog entries, but in my opinion > > > there > > > are quite effective "executive summaries" for the patches and helpful to > > > the > > > reader/reviewer. Please let's not throw the baby with the bath's water. > > > > If there's a mechanism to filter checkin messages to ChangeLog summaries, > > I would be happy to use it - in cases of multiple packages, especially, > > it's > > important to know what changes were made, when, and when the changes > > propagated > > through packages and releases, and where they got to, occasionally. Anybody > > know of a useful, built-in mechanism for this task? > > > > Personally I find it slow and inefficient tracing through why a given > change was made. It is just a slow process searching and sometimes I > don't bother because it is so inconvenient. The ChangeLog entries > provide little help and there does not seem to be a good alternative. If > there is a good alternative no-one has said what it is so far. > > As people have pointed out, the RCSs pretty well cover the "what" these > days. And writing changelog entries, which largely duplicate this > information, is time-consuming and tedious. And there are of of little > to no value to me at least. > > The coding standards do allow, in some cases, that giving some context > would be useful: > > > See also what the GNU Coding Standards have to say about what goes in > > ChangeLogs; in particular, descriptions of the purpose of code and > > changes should go in comments rather than the ChangeLog, though a > > single line overall description of the changes may be useful above the > > ChangeLog entry for a large batch of changes. > > I personally would strongly favour each ChangeLog entry having a single > line of context. This could be the PR number or a single line giving the > purpose of the change or what bigger change it is part of. > > As pointed out by Zach Weinberg in his paper "A Maintenance Programmer's > View of GCC", there are many impediments to contributing to GCC. > > http://www.linux.org.uk/~ajh/gcc/gccsummit-2003-proceedings.pdf > > Things are not much better than they were when Zach wrote his paper. This > small change would be one positive step n the right direction, IMHO. One line of reference would be sufficient provided that branches other than the main development trunk stick to revisions in that branch only. I haven't glanced through the references yet, but maintenance programming is considerably different than writing new code, or even modifying someone else's code. If it's the latter you're trying to achieve, or anticipate achieving, then an accurate line of reference would be most helpful. Unfortunately, then, _someone_ has to maintain the comments accurately. I wouldn't care to say who (whom?)... just... someone. :) -- Ctalk Home Page: http://ctalk-lang.sourceforge.net
Re: Rant about ChangeLog entries and commit messages
> > I'd go even further, and say if the GNU coding standards say we > > shouldn't be putting descriptions of why we are changing things in the > > ChangeLog, than they should be changed and should be ignored on this > > point until they do. Pointing to them as the if they are The One True > > Way seems very suspect to me. After all, how else would they ever > > improve if nobody tries anything different? > > The people who wrote them presumably thought about these issues, too. My understanding is that the concern in going the other way was in having a ChangeLog that was too long to easy scan. Now yes, it's true that the concept of scanning a ChangeLog rather than a CM log quite dated at this point, but that's GNU coding standards issue, not a GCC issue and I don't think that trying to change that will produce much more than heat. I do, however, think that we have significant flexibility in content of the svn commit message and could well decide that it's useful to do more than echo the ChangeLog entry, but instead could include most of the text of the patch submission message.
Re: Rant about ChangeLog entries and commit messages
> Having said that, I find the lack of rationale for some changes to be a > bit irritating. I know that this should be done through code comments, > but those are often made across the changeset and in different files. And it's often not appropriate to put the reason (or even nature) of the change in comments because you run the risk of creating what is essentially a useless monologue to somebody trying to understand the present code: /* Extract the two operands of the expression. Note that at one point this code had a typo and extracted the same operand twice. Paul tried to fix it on February 10, 1997, but actually introduced another bug where it again extracted the same operand twice, but this time the other operand. Jeff finally got it right on February 12, 1997 and that's the present code. */ op0 = TREE_OPERAND (t, 0); op1 = TREE_OPERAND (t, 1); That's not very useful!
Re: Rant about ChangeLog entries and commit messages
> If all the changelog entry says is something like > > (xyz): new function > > I don't see much point, since a diff can always easily tell > you *what* was changed. The point is that, by just looking at the ChangeLog, you can tell when xyz was introduced and by whom. I've used that quite a number of times. Moreover, as was pointed out, when you have a source distribution, you don't get the commit logs, just the ChangeLog.
Re: Rant about ChangeLog entries and commit messages
On 12/2/07, Richard Kenner <[EMAIL PROTECTED]> wrote: > > If all the changelog entry says is something like > > > > (xyz): new function > > > > I don't see much point, since a diff can always easily tell > > you *what* was changed. > > The point is that, by just looking at the ChangeLog, you can tell when > xyz was introduced and by whom. I've used that quite a number of > times. Moreover, as was pointed out, when you have a source > distribution, you don't get the commit logs, just the ChangeLog. There are in fact, already programs that will generate GNU format changelogs from svn log (see http://ch.tudelft.nl/~arthur/svn2cl/) It would be very easy to run these as part of the release process. Last time I looked, this is in fact how some GNU projects generate ChangeLog for distribution! >
Re: Rant about ChangeLog entries and commit messages
> In the Ada revision histories, we have always given the what-and-the-why > (and if necessary the why-not), and they have proved very helpful, I > always found the RH's for gigi (done in the gcc style, much less helpful > because they omitted the why). Sorry, that's simply not true, the ChangeLog and the commit messages in the ada/ subdirectory are on par with the rest of the GCC tree, and that's fine. -- Eric Botcazou
Re: Rant about ChangeLog entries and commit messages
> If you have a better justification, i'd love to hear it. Justification for what? I only tried to explain to Sam why we do things the way we do, I didn't write the GNU Coding Standards either. -- Eric Botcazou
Re: Rant about ChangeLog entries and commit messages
On Sun, 2 Dec 2007, Andreas Schwab wrote: | 2007-11-30 Jan Hubicka <[EMAIL PROTECTED]> | | * ggc-common.c (dump_ggc_loc_statistics): Reset ggc_force_collect | flag. How could a newcomer guess why the gcc_force_collect flag needs to be reset? That is supposed to be written in a comment. Indeed. Some advice I once wrote: Often I see a commit with a log message that lovingly explains a small change made to fix a subtle problem, but adds no comments to the code. Don't do this! Put that careful description in a comment, where people can actually see it. (Commit logs are basically invisible; even if they are auto-emailed to all developers, they are soon forgotten, and they don't benefit people not on the email list.) That comment is not a blemish but an invaluable record of an unusual case that someone didn't anticipate. If the bug-fix was pre-empted by a lengthy email exchange, include some or all of that exchange if it helps. Nick
