Re: Loop unroll fixes

Re: Loop unroll fixes

[mike stump]

> Date: Tue, 18 Sep 2001 10:19:02 +0100 (BST)
> From: "Joseph S. Myers" <jsm28@cam.ac.uk>

> * Where <URL: http://gcc.gnu.org/ml/gcc/2001-05/msg01273.html > says
> (of creating synthetic testcases for bugs shown up by confidential
> code) "sometimes specific tests for fixed bugs are definitely cases
> of closing the doors after the cows have fled", this is misleading
> since such testcases may be necessary in future to debug problems
> with the fix; so creating such testcases would not be as described
> there "work that competes with everything else going on" but
> something at which a reasonable effort *must* be made before a fix
> is installed in FSF GCC.

I think that all changes to gcc should come with at least one testcase
that actually demonstrates why the change was a good idea.  If the
code merely changes performance, then the performance suite should
have a testcase that shows the performance advantage.  The last point
we currently never do, so I can be dissuaded for now that it is
unrealistic, but, I think for real bugs, it is possible to meet this
requirement.  The testsuite is invaluable to the quality of the
compiler in so many ways.  It should be a requirement, not optional.

I do realize that sometimes a testcase isn't possible, but in my
experience, those are rare.  A good example of such, would be a
testcase in the C++ framework to test throwing in/out and trough a
shared library.  The old-deja.exp framework doesn't have enough beef
to do this.  A testcase is impossible within it, without massive
modifications to old-deja.exp.

An non-example would be 112 thousand lines of customer proprietary
code which changes any time almost anything is played with.  Been
there, done that, not that hard, just time consuming to trim the
testcase and sanitize it.

If the folks that contributed the testsuite we have felt the same way
as dewar, it is clear to me that we would not have a gcc/g++
testsuite, or that it would be smaller.

Re: Loop unroll fixes

[Joseph S. Myers]

On Tue, 18 Sep 2001, mike stump wrote:

> I think that all changes to gcc should come with at least one testcase
> that actually demonstrates why the change was a good idea.  If the

In the case of some code cleanup and reorganisation patches (that don't
change user-visible behaviour), of course, there aren't appropriate
testcases (apart possibly from a reduction of the number of warnings
during bootstrap, or an improvement in compile-time performance).

> An non-example would be 112 thousand lines of customer proprietary
> code which changes any time almost anything is played with.  Been
> there, done that, not that hard, just time consuming to trim the
> testcase and sanitize it.

A harder case might be where the bug was shown up by a small testcase from
a proprietary testsuite (see e.g.  
<URL: http://gcc.gnu.org/ml/gcc-patches/1998-11/msg00257.html >).  Do we
have any legal guidance on what is and is not safe to do in such cases?

-- 
Joseph S. Myers
jsm28@cam.ac.uk

Re: Loop unroll fixes

[Joe Buck]

Joseph Myers writes:
> A harder case might be where the bug was shown up by a small testcase from
> a proprietary testsuite (see e.g.  
> <URL: http://gcc.gnu.org/ml/gcc-patches/1998-11/msg00257.html >).  Do we
> have any legal guidance on what is and is not safe to do in such cases?

We got some legal advice from Eben Moglen (FSF counsel) on test suite
issues.  We could perhaps ask him for advice on how to legally create a
test case that we can distribute.  But the following procedure is
generally considered safe:

programmer A, with access to the test case, writes up a report on what
the test is doing and what the failure is.

(for the extremely paranoid: lawyer reads report, sprinkles holy water,
but probably unneeded in this case)

programmer B uses the report to write a test case that shows the same
failure.

Re: Loop unroll fixes

[law]

  In message < 200109191827.LAA19330@atrus.synopsys.com >you write:
  > Joseph Myers writes:
  > > A harder case might be where the bug was shown up by a small testcase fro
  > m
  > > a proprietary testsuite (see e.g.  
  > > <URL: http://gcc.gnu.org/ml/gcc-patches/1998-11/msg00257.html >).  Do we
  > > have any legal guidance on what is and is not safe to do in such cases?
  > 
  > We got some legal advice from Eben Moglen (FSF counsel) on test suite
  > issues.  We could perhaps ask him for advice on how to legally create a
  > test case that we can distribute.  But the following procedure is
  > generally considered safe:
  > 
  > programmer A, with access to the test case, writes up a report on what
  > the test is doing and what the failure is.
  > 
  > (for the extremely paranoid: lawyer reads report, sprinkles holy water,
  > but probably unneeded in this case)
  > 
  > programmer B uses the report to write a test case that shows the same
  > failure.
Right.  In fact, this kind of discipline (IMHO) would generally lead to 
more thorough bug analysis and hopefully fewer incorrect patches.

jeff

Re: Loop unroll fixes

[Mark Mitchell]

--On Wednesday, October 10, 2001 02:12:34 PM +0200 Franz Sirl 
<Franz.Sirl-kernel@lauterbach.com> wrote:

> At 10:16 10.10.2001, Mark Mitchell wrote:
>
>
>> --On Wednesday, October 10, 2001 10:12:38 AM +0200 Wolfgang Bangerth
>> <wolfgang.bangerth@iwr.uni-heidelberg.de> wrote:
>>
>>>
>>>> I anticipate those being the last changes for GCC 3.0.2.
>>>
>>> Not arguing for this just because it happened to bite me, but it might
>>> be worth to also look at c++/4512, which is a regression w.r.t. 3.0.1.
>>> Franz said he'll look into it.
>>
>> Thank you for pointing this out.  We need to identify this problem, fix
>> it, or remove the patch that is causing it.  Regressions from previous
>> 3.0 releases are very, very bad.
>
> I already tracked down what's happening and produced a simple fix.
> Bootstrapped and regtested on x86 so far.

It looks a little hokey (no offense intendend; you're doing great under
tough constraints), but again, I can't see how it can hurt.  So, go
ahead and check it in as soon as possible.

Thank you,

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: Loop unroll fixes

[Franz Sirl]

[-- Attachment #1: Type: text/plain, Size: 1659 bytes --]

At 10:16 10.10.2001, Mark Mitchell wrote:


>--On Wednesday, October 10, 2001 10:12:38 AM +0200 Wolfgang Bangerth 
><wolfgang.bangerth@iwr.uni-heidelberg.de> wrote:
>
>>
>>>I anticipate those being the last changes for GCC 3.0.2.
>>
>>Not arguing for this just because it happened to bite me, but it might be
>>worth to also look at c++/4512, which is a regression w.r.t. 3.0.1. Franz
>>said he'll look into it.
>
>Thank you for pointing this out.  We need to identify this problem, fix
>it, or remove the patch that is causing it.  Regressions from previous
>3.0 releases are very, very bad.

I already tracked down what's happening and produced a simple fix. 
Bootstrapped and regtested on x86 so far.

The problem here is that if a loop gets unrolled, the uid_luid array 
doesn't get updated. In the testcase the loop getting unrolled is part of 
the loop exit tests of another loop. So when the loop exit insns are 
scanned as in the last change to loop_iterations, we get an abort as soon 
as we hit an insn that was generated by loop unrolling :-(.

Probably the cleanest solution would be to recalculate uid_luid after each 
call to scan_loop. Since this wasn't done so far, I guess this has either 
compile time impacts or would confuse other parts of the loop code.
Fortunately, in this case we can just ignore the insns generated by loop 
unrolling, cause they will contain the same insns than the original loop 
body and it's enough if we test for back edges only in the original ones.

So I suggest the appended patch.

OK to commit?

Franz.

         PR c++/4512
         * unroll.c (loop_iterations): Ignore insns generated by loop 
unrolling.


[-- Attachment #2: gcc3-loop-2.patch --]
[-- Type: text/x-diff, Size: 668 bytes --]

Index: gcc/unroll.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/unroll.c,v
retrieving revision 1.125.4.2
diff -u -p -r1.125.4.2 unroll.c
--- gcc/unroll.c	2001/10/08 21:33:16	1.125.4.2
+++ gcc/unroll.c	2001/10/10 12:08:05
@@ -3506,6 +3523,11 @@ loop_iterations (loop)
 
       do
 	{
+	  /* Previous unrolling may have generated new insns not covered
+	     by the uid_luid array.  */
+	  if (INSN_UID (temp) >= max_uid_for_loop)
+	    continue;
+
 	  if (GET_CODE (temp) == JUMP_INSN
 	      && INSN_LUID (JUMP_LABEL (temp)) > INSN_LUID (loop->top)
 	      && INSN_LUID (JUMP_LABEL (temp)) < INSN_LUID (loop->cont))

Re: Loop unroll fixes

[Mark Mitchell]

--On Wednesday, October 10, 2001 10:12:38 AM +0200 Wolfgang Bangerth 
<wolfgang.bangerth@iwr.uni-heidelberg.de> wrote:

>
>> I anticipate those being the last changes for GCC 3.0.2.
>
> Not arguing for this just because it happened to bite me, but it might be
> worth to also look at c++/4512, which is a regression w.r.t. 3.0.1. Franz
> said he'll look into it.

Thank you for pointing this out.  We need to identify this problem, fix
it, or remove the patch that is causing it.  Regressions from previous
3.0 releases are very, very bad.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: Loop unroll fixes

[Wolfgang Bangerth]

> I anticipate those being the last changes for GCC 3.0.2.

Not arguing for this just because it happened to bite me, but it might be
worth to also look at c++/4512, which is a regression w.r.t. 3.0.1. Franz
said he'll look into it.

Regards
  Wolfgang


-------------------------------------------------------------------------
Wolfgang Bangerth          email: wolfgang.bangerth@iwr.uni-heidelberg.de
                             www: http://gaia.iwr.uni-heidelberg.de/~wolf

Re: Loop unroll fixes

[Mark Mitchell]

--On Friday, October 05, 2001 01:10:05 AM -0500 Zoltan Hidvegi 
<hzoli@hzoli.2y.net> wrote:

>> > There are three testcases for Zoltan's 3 patches.  Two of these
>> > testcases are not regressions.  They fail at -O2 -funroll-loops in
>> > both gcc-2.95.2 and gcc-3.0.1.  The bugs should be fixed in mainline
>> > of course, but I see no critical need to include them in gcc 3.0.2.
>

Would you please send me your most current version of these patches,
including only those that fix bugs that are regressions from GCC 2.95,
and with examples showing what bugs they fix?

I will review them, and hopefully check them in, tomorrow morning.

I anticipate those being the last changes for GCC 3.0.2.

Thank you,

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: Loop unroll fixes

[Zoltan Hidvegi]

> > There are three testcases for Zoltan's 3 patches.  Two of these testcases
> > are not regressions.  They fail at -O2 -funroll-loops in both gcc-2.95.2
> > and gcc-3.0.1.  The bugs should be fixed in mainline of course, but I see
> > no critical need to include them in gcc 3.0.2.

Oh, that only because I had one testcase catching 2 bugs.  How about this:

------------------- BEGIN ---------------
void abort(void);
int
sum(int m)
{
    int i;
    int s = 0;

    for (i = 0; i < m; i += 2)
	s += i;

    return s;
}

int
main(void)
{
    if (sum(15) != 56)
	abort();
    return 0;
}
------------------- END ----------------

That works on 2.95.2, but fails on 3.0.x.  In fact, that's the most
serious one, that's the one that coredumped the program I'm working
on.  I was quite busy with work recently, but I may have some more
time coming up.

Zoli

Re: Loop unroll fixes

[Mark Mitchell]

> There are three testcases for Zoltan's 3 patches.  Two of these testcases
> are not regressions.  They fail at -O2 -funroll-loops in both gcc-2.95.2
> and gcc-3.0.1.  The bugs should be fixed in mainline of course, but I see
> no critical need to include them in gcc 3.0.2.

Thank you for your analysis.  I agree: only the regression needs to be
fixed in 3.0.2.

> enough, to be included in gcc-3.0.2.  I am willing to approve this patch
> for mainline and gcc-3.0.2. This is the patch in
> 	http://gcc.gnu.org/ml/gcc-patches/2001-09/msg00554.html

Great.  Would you mind applying it?

> I need to spend more time looking at the other two patches.  If we still
> want these patches in gcc-3.0.2, then the review will take longer than if
> putting them in mainline is OK, since I need to be more careful with
> patches for gcc-3.0.2.

We don't need them in 3.0.2.

> PS The testcase from Franz Sirl is a regressison at -O2, and hence is a
> more important problem than any of the 3 testcases from Zoltan which all
> require -O2 -funroll-loops to trigger.

Yes, I agree.

Thanks,

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: Loop unroll fixes

[Jim Wilson]

In article < 5.1.0.14.2.20011004153251.054de990@mail.lauterbach.com > you write:
>Mark, Bernd, what about this for mainline and 3.0.2? Somehow the patch 
>never made it past an half-approved state. I stumbled over this while 
>tracking another (unfortunately unrelated, more info later today) loop bug.

This is partly my fault.  I was supposed to finish the patch review that
Bernd started.

There are three testcases for Zoltan's 3 patches.  Two of these testcases
are not regressions.  They fail at -O2 -funroll-loops in both gcc-2.95.2 and
gcc-3.0.1.  The bugs should be fixed in mainline of course, but I see no
critical need to include them in gcc 3.0.2.

The third one is a regression.  It fails at -O2 -funroll-loops in gcc-3.0.1,
but works with the same options in gcc-2.95.2.  This testcase is
void
do_loop(unsigned long c, char *m)
{
    unsigned long i = 0;

    do {
        m[i] = 0;
    } while (++i != c);
}
This one is fixed by the small doloop.c patch, which happens to be the one
patch that Bernd did review.  There is a rather nice review in
	http://gcc.gnu.org/ml/gcc-patches/2001-09/msg00596.html
I agree with Bernd's analysis.  The submitted patch doesn't fix the underlying
problem, but it does have value since it optimizes away unnecessary code, and
this optimization helps workaround the underlying bug for some testcases.
As such, I think it has enough value, and is safe enough, to be included in
gcc-3.0.2.  I am willing to approve this patch for mainline and gcc-3.0.2.
This is the patch in
	http://gcc.gnu.org/ml/gcc-patches/2001-09/msg00554.html

I need to spend more time looking at the other two patches.  If we still
want these patches in gcc-3.0.2, then the review will take longer than if
putting them in mainline is OK, since I need to be more careful with patches
for gcc-3.0.2.

Jim

PS The testcase from Franz Sirl is a regressison at -O2, and hence is a
more important problem than any of the 3 testcases from Zoltan which all
require -O2 -funroll-loops to trigger.  However, the Zoltan patches are
already 3 months old, so I don't want to delay their review any longer.

Re: Loop unroll fixes

[Mark Mitchell]

> OK to commit Zoltans patch to mainline (after he sends me an updated
> mainline version) and branch after bootstrap/regtest on x86/ppc?

Yes -- if Bernd approves.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: Loop unroll fixes

[Franz Sirl]

At 01:35 14.09.2001, Zoltan Hidvegi wrote:
>Sorry to bug you all with that again, but I wonder if the fix for PR
>3384 could be applied to the 3.0 branch and the mainline (on the
>mainline the doloop.c patch conflicts with Bernd Smith's change to fix
>a subset of the bugs fixed by my patches that also generates a slighly
>less efficient code).  See PR 3384, and threads
> http://gcc.gnu.org/ml/gcc-patches/2001-07/msg01080.html
> http://gcc.gnu.org/ml/gcc-bugs/2001-07/msg00750.html
>
>I think this is a bad regression from 2.95.2.  -funroll-loops is a
>powerful and widely used optimization.  GNATS already have a high
>priority assigned to this bug, please, someone look at it and either
>check it in or do some alrernative fix.  Bootstrapped and passed the
>regressions on powerpc-ibm-aix4.3.3 and on i686-pc-linux-gnu, at least
>it did some time back just before 3.0.1 was released, but there has
>been no change in the files involved since than.

Mark, Bernd, what about this for mainline and 3.0.2? Somehow the patch 
never made it past an half-approved state. I stumbled over this while 
tracking another (unfortunately unrelated, more info later today) loop bug.

OK to commit Zoltans patch to mainline (after he sends me an updated 
mainline version) and branch after bootstrap/regtest on x86/ppc?

Franz.

Re: Loop unroll fixes

[Richard Kenner]

    I'd expect that common configurey for all targets and front ends are
    affected by this, making this as much of a major feature as say, the
    C++ or Java FE's.

I don't follow your first point, but would certainly also not consider the
Java or C++ "major features" using the relevant definition, which isn't
how much it adds to the compiler, but relates instead to the chances of
it breaking something else.

Re: Loop unroll fixes

[Benjamin Kosnik]

> I don't see that as a "major feature", but rather the same as a new
> backend, which can be dropped in right up to the last minute, since no
> common files are affected.

Huh?

I'd expect that common configurey for all targets and front ends are
affected by this, making this as much of a major feature as say, the
C++ or Java FE's.

-benjamin

Re: Loop unroll fixes

[Richard Kenner]

    * We should get Ada into the tree as soon as possible (in particular,
    before the cut-off on October 15 for major new features for 3.1), 

I don't see that as a "major feature", but rather the same as a new backend,
which can be dropped in right up to the last minute, since no common files
are affected.  That being said, it's obviously best to get it done as soon as
possible, hopefully before that date, but right now ACT is off the net due to
communication problems caused by the WTC attack.

    since sometimes these sorts of problems have occurred with patches
    written to fix bugs showing up with Ada testcases.  

I don't understand that comment.  The vast majority of the Ada testcases are
prioprietary, though bootstrapping the compiler itself and some of the test
cases are still useful.

    What caused the delay from the date of June 1 given in
    <URL: http://gcc.gnu.org/ml/gcc/2001-05/msg01148.html >?

It being bumped by other work plus a vacation of the person who was going to
do it, plus a company meeting, etc.

Re: Loop unroll fixes

[Joseph S. Myers]

On Mon, 17 Sep 2001, Richard Henderson wrote:

> Suppose the patch is wrong in some way that causes it to fail for
> some obscure target under some conditions.  Suppose this is not
> discovered for a year.  If the patch is well documented, one can
> quickly recall what the original problem was, as opposed to either
> (1) spending lots of time re-examining the original bug, or
> (2) guessing the that the old patch was wrong and reverting it.
> 
> Both (1) and (2) have happened many times in gcc history.

There should have been a testcase added for the original problem.

Two consequences of this:

* We should get Ada into the tree as soon as possible (in particular,
before the cut-off on October 15 for major new features for 3.1), since
sometimes these sorts of problems have occurred with patches written to
fix bugs showing up with Ada testcases.  What caused the delay from the
date of June 1 given in
<URL: http://gcc.gnu.org/ml/gcc/2001-05/msg01148.html >?

* Where <URL: http://gcc.gnu.org/ml/gcc/2001-05/msg01273.html > says (of
creating synthetic testcases for bugs shown up by confidential code)
"sometimes specific tests for fixed bugs are definitely cases of closing
the doors after the cows have fled", this is misleading since such
testcases may be necessary in future to debug problems with the fix; so
creating such testcases would not be as described there "work that
competes with everything else going on" but something at which a
reasonable effort *must* be made before a fix is installed in FSF GCC.

-- 
Joseph S. Myers
jsm28@cam.ac.uk

Re: Loop unroll fixes

[Mark Mitchell]

--On Monday, September 17, 2001 03:11:42 PM -0700 Richard Henderson 
<rth@redhat.com> wrote:

> On Mon, Sep 17, 2001 at 02:24:32PM -0700, Joe Buck wrote:
>> So how about requiring such documentation as part of the patch?
>
> Fine by me.

Me, too.  I think everyone should play by the same rules, even those
of us with global write privileges.

> In addition, it aids debugging future problems as well.
>
> Suppose the patch is wrong in some way that causes it to fail for
> some obscure target under some conditions.  Suppose this is not
> discovered for a year.  If the patch is well documented, one can
> quickly recall what the original problem was, as opposed to either
> (1) spending lots of time re-examining the original bug, or
> (2) guessing the that the old patch was wrong and reverting it.
>
> Both (1) and (2) have happened many times in gcc history.

Yes.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: Loop unroll fixes

[Richard Henderson]

On Mon, Sep 17, 2001 at 02:24:32PM -0700, Joe Buck wrote:
> So how about requiring such documentation as part of the patch?

Fine by me.

> but at least some explanation of what is going wrong, what the patch does,
> and why this is the right thing would make patch reviewers' lives easier
> and probably improve the quality of gcc.

In addition, it aids debugging future problems as well.

Suppose the patch is wrong in some way that causes it to fail for
some obscure target under some conditions.  Suppose this is not
discovered for a year.  If the patch is well documented, one can
quickly recall what the original problem was, as opposed to either
(1) spending lots of time re-examining the original bug, or
(2) guessing the that the old patch was wrong and reverting it.

Both (1) and (2) have happened many times in gcc history.


r~

Re: Loop unroll fixes

[Joe Buck]

On Sat, Sep 15, 2001 at 04:56:28PM +0100, Bernd Schmidt wrote:
> > I've complained about missing documentation for the patches, so let me try
> > to suggest how this patch should have been described:

Richard writes:
> Oh were every patch documented like you describe!  It would
> make the review process _so_ much easier.

So how about requiring such documentation as part of the patch?  We might
not require that it be quite so thorough as Bernd's example (though we
might put his example up on the web as a model of what we'd like to see),
but at least some explanation of what is going wrong, what the patch does,
and why this is the right thing would make patch reviewers' lives easier
and probably improve the quality of gcc.

Then if we combine that with my earlier suggestions, some less experienced
folks could sign up to make sure that these basic criteria are met
(pre-filtering), and gurus like Richard wouldn't be asked to waste their
time on patches that don't meet standards.

Joe

Re: Loop unroll fixes

[Richard Henderson]

On Sat, Sep 15, 2001 at 04:56:28PM +0100, Bernd Schmidt wrote:
> I've complained about missing documentation for the patches, so let me try
> to suggest how this patch should have been described:

Oh were every patch documented like you describe!  It would
make the review process _so_ much easier.


r~

Re: Loop unroll fixes

[Bernd Schmidt]

Before we close this flame war, let my try to add something constructive.

On Fri, 14 Sep 2001, David Edelsohn wrote:

> 	The following has nothing to do with unrolling.  A do-while loop
> with != condition is generated wrong.
>
> extern int m[];
> int i = c;
>
> do {
>   m[i] = 0;
> } while (++i != c);
>
> performs one, fixed iteration on PowerPC, never comparing i and c.  The
> reality is GCC cannot determine when this loop will terminate.  GCC
> applies the "bogus iteration count" rule to set the iteration count to
> one, which does not apply to this type of test.

I've complained about missing documentation for the patches, so let me try
to suggest how this patch should have been described:

  The doloop.c transformation tries to change the exit test of the loop.
  It computes (or emits code to compute) the number of iterations, preloads
  this number into a register, and changes the loop so that this register
  is used as a counter.

  (The following is slightly simplified to deal only with loops where the
  increment is 1.  It should apply equally to other kinds of loops.)

  Normally, the iteration count is equal to (final_value - initial_value) of
  the register used as the loop counter before the transformation.  However,
  it is possible that the exit condition is true immediately (e.g. in the
  case
    i = 5;
    do {} while (i < 4);
  In the case of a do-while loop, this means that the loop is executed
  exactly once, so we must make sure that we take this into account when
  computing the number of iterations.  At the end of doloop_modify_runtime
  there is code that will emit code to detect this case and set the
  iteration count to 1 at runtime if we find that we have a loop of this
  form.

  This fails for the testcase [*]

  extern int m[];
  int i = c;

  do {
    m[i] = 0;
  } while (++i != c);

  The new test we emit before the loop sets the iteration count to 1, which
  is wrong.
  [**] We do not need this kind of test for a NE type loop, since the exit
  condition can only become true if the loop is entered with
    (final_value - initial_value) == 1,
  but in that case, we don't need to explicitly set the iteration count to
  1, since the earlier code has already done that.

Now, let's discuss this.  A few notes first:
[*]  A better testcase would be something that can be compiled and run.
     The given testcase is not a complete program, and if it were, it would
     segfault.  It's also invalid since there's a signed integer overflow
     involved.
[**] This is where there's a fault in the logic.  The question that needs to
     be asked is "why doesn't the added test do the right thing", not "can
     we make it go away in this case"?  These are separate questions, and to
     fix the bug, you have to answer the first.

The missing piece to replace the part marked [**] would read something like
  It fails because it's wrong to compare the initial value to the final
  value - at the end of the first iteration, the loop counter (I) has
  already been incremented once, so the comparison we emit is not the same
  one that would happen naturally - it compares two different values.  For
  this and similar loops, we must increment the initial value once before
  doing the comparison.

When submitting a patch, it is always a good idea to give a kind of
"walkthrough".  Submitters should never assume that even a person who is
listed as the maintainer of a particular area has ever looked at the
specific piece of code that is being changed.  GCC is too big for this.
People should make sure that their explanation contains all the relevant
facts (e.g. in this case, why do we emit the extra test anyway - what
does it do?)

If that information is missing, the review will become harder, since
the reviewer will have to duplicate the analysis for h{im,er}self.  It's
easier to follow a well-written argument and to only make sure that every
logical step is correct.  It should also help the patch writer - if it's
hard to write down a logical argument, that may mean that the patch isn't
correct.

For me, this sort of thing can help reduce the time required for review
from hours to minutes (if it makes any sense - not if it contains flaws
in the reasoning, of course).  Of course, it puts additional burden on
the submitters, but it's IMO time better spent than additional testing on
a zillion of platforms - that won't detect problems like this one.


Bernd

>  Index: doloop.c
>  ===================================================================
>  RCS file: /cvs/gcc/gcc/gcc/doloop.c,v
>  retrieving revision 1.3.4.1
>  diff -u -r1.3.4.1 doloop.c
>  --- doloop.c   2001/05/12 20:32:26     1.3.4.1
>  +++ doloop.c   2001/07/16 21:09:42
>  @@ -661,7 +634,7 @@
>        not executed before the start of the loop.  We need to determine
>        if the loop will terminate after the first pass and to limit the
>        iteration count to one if necessary.  */
>  -  if (! loop->vtop)
>  +  if (! loop->vtop && comparison_code != NE)
>       {
>         rtx label;
>

Re: Loop unroll fixes

[Joseph S. Myers]

On Fri, 14 Sep 2001, Jim Wilson wrote:

> I don't know how to improve on that without hiring
> someone to do the task, and we don't have money for that.

I accept that hiring someone for patch review wouldn't be a good use of
FSF funds, but could you (or, if you don't have them, someone in a
convenient position to obtain them from the Secretary of State of the
Commonwealth of Massachusetts per
<URL: http://groups.google.com/groups?hl=en&selm=u1hwwaya3b3.fsf%40pusey.MIT.EDU >)
make the FSF financial accounts that back up that statement that the funds
aren't available available online or point to where they are online if
they already are?

-- 
Joseph S. Myers
jsm28@cam.ac.uk

Re: Loop unroll fixes

[Bernd Schmidt]

On Fri, 14 Sep 2001, David Edelsohn wrote:
>
> 	The intent of my original message was to notify everyone that I
> was not going to allow the bottleneck on this patch to continue.

What you overlooked was that you have no authority to do so.  You
neither have the authority to check in an unreviewed patch, nor the
authority to decide for others whether they view a patch as high
priority or not.


Bernd

Re: Loop unroll fixes

[Jim Wilson]

>	Please keep in mind that your messages state your opinion of the
>events, not some objective facts.

I have made many statements of fact, and I have tried to provide
documentation for all of them.  You have not provided any verifiable
documentation to contest any of them.

There are two facts of particular notice
1) You have made accusations of malfeasance against other gcc developers,
   but you have not provided any verifiable evidence to back these claims.
2) You stated that you would deliberately violate gcc process, but you have
   not provided any verifiable evidence to prove that this was necessary.

I have also provided opinions, you are welcome to disagree with them, but
that does not change the facts.

> I only can express my view of the events as well. 

I have not seen you provide any verifiable documentation for any of your
claims.

> I respect your opinion, although
>I think you developed your conclusions before investigating all of the
>information.

This is wrong.  I researched every shred of evidence that I could find on
gcc.gnu.org.  I looked at the PR in question, 3384.  I searched every mailing
list for the PR number, for Zoltan Hidvegi's name, and I followed every
thread I could find.  The only information I did not investigate was
personal interviews of the involved parties, but I did not have time for
that, because you had specified too short of a deadline to allow for that.

I included all of this evidence in my original message on this thread.
The wealth of references in my message make it clear that I invested a
considerable amount of time investigating the issues.  For reference, my
message is here
http://gcc.gnu.org/ml/gcc/2001-09/msg00533.html

I see no basis for your opinion that my investigation was insufficient.
If you can provide verifiable evidence that I am wrong, then please do so.

>	The intent of my original message was to notify everyone that I
>was not going to allow the bottleneck on this patch to continue.  I did
>not intend for anyone to immediately review the patch or generate
>alternate fixes for bugs highlighted by the patch. 

I contest this claim.  Your original message is here:
http://gcc.gnu.org/ml/gcc/2001-09/msg00529.html
In this message, you say:
If there is no specific, documented, technical objection to this
patch within one week, it will be approved.

You could have achieved your goal by reminding everyone that the patch was
still unreviewed, and that you wanted the patch reviewed in time to be
included in gcc 3.0.2.

Instead, you issued an ultimatum.  You specified a deadline, and you stated
that you would take unilateral action in contravention of documented gcc
policies if there was no objection.  This was designed to provoke an immediate
review of the patch, as that is the only way to generate a technical objection
to the patch.

As supporting evidence, I offer a quote from this message:
http://gcc.gnu.org/ml/gcc/2001-09/msg00549.html
I quote:
Your vindictive response of further delaying the patch does not
help GCC or anyone.

If you were not interested in an immediate review of the patch, then the
fact that I said I would look at it in one week's time should not have been
a serious problem.  I will admit this one is weak, one could argue that
you were contesting the style of the response, and not the delay, but still,
I think it has merit as a supporting claim.  After all, I did agree to review
the patch, I just did not agree to review it immediately.

> None of us has any sort of special
>privilege or insight to decree what is right or what is wrong or what is
>sufficient evidence or what policy is appropriate.

And this is exactly why I objected to your original message.  Because with
your ultimatum, you were claiming special priviledges for yourself to set
policy.  You have stated this much better than I did.

And with that, I'd like to end this thread.  If David wants a rebuttal,
I won't object.  I would suggest that anyone who wants to help solve the
problem of patch reviewing contribute to Joe Buck's thread instead of this
one.  I will followup on Zoltan's patch as I promised.  I am at the moment
waiting to see what comes from Bernd's review.

As a final word, I will admit that I am guilty of unnecessary flamage, but
I thought the issues raised here were worth the risk.

Jim

Re: Loop unroll fixes

[Daniel Berlin]

Jim Wilson <wilson@cygnus.com> writes:

>>No, I'd rather believe he's trying to just bully people into reviewing
>>a patch.
>>Which is good, in reality.
>
> I can understand that you are probably responding in frustration, perhaps from
> personal experience with problems with patch reviews, but I still don't think
> that justifies a lack of civility in this case.
Errr, i'm confused which lack of civility you are referring to, yours,
mine, or his?

>
> If there was evidence in the public record that a maintainer was unresponsive,
> then yes, bullying would be appropriate.  But bullying should not be the
> first approach, it should be the last resort.

And at what point do you consider it a last resort?
Days?
Weeks?
Months?
Years?

>   There is no public evidence
> to indicate that an maintainer was unresponsive.  And there is public evidence
> indicating that bullying was the first approach used.
>
>>It shouldn't take months to review patches.
>
> Ideally, yes, but we get so many patches that no matter how good the process
> is, there will always be some patches that are delayed.
Having an insignificant number of patches (Way less than 1%) that are
delayed is an achievable goal.

> majority of patches get a response in a reasonable amount of time.

I'm not so sure about this, but i'll reserve judgement for another
year or so.
>   When
> someone points out unreviewed patches, they usually get a response in a
> reasonable amount of time.
Same here.
>   Most patches that are delayed after that are
> controversial and/or potentially disruptive. 
I disagree, currently, but once again, i'll reserve judgement for
another year or so.
> That is not unreasonable.
If it were true, i would agree it's not unreasonable.
> I agree that the current process is a pain to work with, but overall I think
> it is working reasonably well.
Same reservation.
>
> In this particular case, I presented evidence showing that there was one
> month delay because of miscommunication, and one more month of delay because
> of a conflict with the gcc 3.0.1 release process.  Thus the two month delay
> in this particular case, though unfortunate, is perfectly understandable,
> and does not indicate any serious flaws with the current system.
>
>>This is the wrong approach.
>>Your job, as a maintainer, is to review patches.
>>It's not some "honor", or exalted position.
>>You are supposed to review every patch, that comes into your area, in
>>a reasonable amount of time.  Months is not reasonable.
>
> I presented ample evidence in my last message to demonstrate that there is
> no basis for this accusation in this particular case.
Accusation?
I said delaying is the wrong approach.
I have not made an accusation or claim, besides that maintainers are
supposed to review patches, regardless of their protest or non-protest.
> Making this claim
> is offensive, and couterproductive.  I did volunteer to review this patch,
> and now my thanks for my trouble is getting a flame from you. 
That's a flame?
I think you've misinterpreted it.

>  If volunteers
> get flamed for trying to help, then they will be less willing to help in the
> future, and we will only have more trouble with patch reviews.
>
>>Deferring action for a week just looks like you are trying to have a
>>fiefdom of your own.
>
> Another offensive comment.
How so?
I'm simply giving you an idea of what it appears like, whether you
like how it appears to me or not. I find it strange you can find a way
to take offense at that.
>   My approach is easy to understand, it is a simple
> application of negative reinforcement.
Which is, again, a bad idea in this case, IMHO.
>   If someone uses threats against me,
> then I will delay cooperating with them. 
>  The more I am threated, the more
> I will delay.
And the worse *you* look.
Not the person doing it.
This is because as a maintainer, it doesn't make squat difference if
the person is an asshole or not, it's still your job to review their
patches in a timely manner.
Once again, IMHO. I'm simply presenting my viewpoint. Don't interpret
it as an accusation.
>  Eventually, I expect people will learn that threats are
> counterproductive, and stop using them.
Doubtful, since the majority are likely occasional contributors, and
when you ignore them, they'll just go away.
>   I am willing to accept the risk
> that this approach will occasionally backfire. 
I argue that this will constantly backfire, and will just decrease the
quality of gcc.
> I will not accept the claim
> that this approach is wrong.
Accept it or not, doesn't change whether it actually *is* the right
approach or not (Same applies to me, of course).
>  After all, I am a volunteer, and I reserve the
> right to decide what I do with the time that I have volunteered to spend on
> gcc.
Sure.  But if a maintainer isn't reviewing patches, or otherwise not
doing their job as a maintainer (once again, neither is an accusation,
i'm simply giving a general statement, not one directed at you), then
they shouldn't expect to be the only once given the ability to review
those patches.
In other words, if a maintainer can't seem to get patches reviewed in
a reasonable amount of time, be it intentional (they were
"threatened", and used your approach), or not, the solution is to add
more maintainers, some who can deal with the "threats".
GCC should not be slowed down, simply because someone decides they are
being threatened. It's bigger than any one person, and not everyone
who contributes will have a likable personality. It would be a mistake
to simply ignore those who don't have one, assuming their patches are
no better or worse than any other contributor.

Once again, most of this is all philosophical argumentation, not one
directly pertaining to the situation at hand.

--Dan
>
> Jim

-- 
"There was a power outage at a department store yesterday.
Twenty people were trapped on the escalators.
"-Steven Wright

Re: Loop unroll fixes

[David Edelsohn]

>>>>> Jim Wilson writes:

Jim> Another offensive comment.  My approach is easy to understand, it is a simple
Jim> application of negative reinforcement.  If someone uses threats against me,
Jim> then I will delay cooperating with them.  The more I am threated, the more
Jim> I will delay.  Eventually, I expect people will learn that threats are
Jim> counterproductive, and stop using them.  I am willing to accept the risk
Jim> that this approach will occasionally backfire.  I will not accept the claim
Jim> that this approach is wrong.  After all, I am a volunteer, and I reserve the
Jim> right to decide what I do with the time that I have volunteered to spend on
Jim> gcc.

	Jim, you have ever right to determine how you expend your
volunteer effort.

	Please keep in mind that your messages state your opinion of the
events, not some objective facts.  I only can express my view of the
events as well.  If you want to reprimand and punish me based on your
interpretation, fine.  Please do not imply that your opinion is more
correct than that of anyone else.  None of us has any sort of special
privilege or insight to decree what is right or what is wrong or what is
sufficient evidence or what policy is appropriate.

	Have you considered that your messages on this thread could be
interpreted as condescending and elicit responses which you would find
offensive?  You are speaking with people who disagree with you, not
training people who are inferior to you.  I respect your opinion, although
I think you developed your conclusions before investigating all of the
information.

	The intent of my original message was to notify everyone that I
was not going to allow the bottleneck on this patch to continue.  I did
not intend for anyone to immediately review the patch or generate
alternate fixes for bugs highlighted by the patch.  I simply did not want
things to remain in limbo.  Zoltan and I always have been ready to address
any concerns about or omissions in the patch, but that requires feedback.

Thanks, David

Re: Loop unroll fixes

[Jim Wilson]

>	Zoltan repeatedly posted messages at various intervals asking
>about the patch.

I addressed this in my first message on this thread, with documentation.
If you wish to contest this, then please make specific objections.
My message is at:
http://gcc.gnu.org/ml/gcc/2001-09/msg00533.html

To recap, Zoltan posted the patch in July, 6 people got involved in the review,
3 people made some claim to having fixed it, and then it died, apparently
because everyone thought someone else was handling the problem.  At the time,
I had sufficient reason to believe that someone else had already fixed the
problem.

Zoltan brought up the issue again in August.  The issue was deliberately
postponed because it conflicted with the gcc 3.0.1 release.

Zoltan brought up the issue again in September.  At this point, you claimed
drastic action was needed.

I see no public evidence that the patch was ignored by anyone, and hence no
support for your attempt to force the issue.  The two month delay is
unfortunate, but understandable given the circumstances.

>My private communication only followed up after each of
>his repeated public requests.

I claim that this was a mistake.  Your use of private communication hid the
fact that there was a problem from the other gcc developers.  If the
followups had been posted to one of the public gcc lists, then others
would have known about the problem, and likely someone would have volunteered
to help.

For example, in one of the first messages I got in July, you indicated that
you and Mark Mitchell would check in the patch.  It was only yesterday, that
I discovered that you did not check in the patch because Bernd said he wanted
to review it.  If that fact had been mentioned on a public mailing list, or
in the PR, then I would have known that the issue had not been resolved yet.
But it appears that the only place this fact was mentioned was in private
communications with others.  Hence it is not surprising that I did not realize
that the issue was still unresolved.

>	I do not accept your statement that the patch and its lack of
>attention was secret.

I never stated that the patch was secret.  I did state that the lack of
attention was secret.  You have produced no evidence to indicate otherwise.
If this claim is wrong, then please point me substantiating evidence on
the gcc.gnu.org site.  I believe I have produced suffficient evidence to
indicate otherwise.  See my mail message referenced above.

>  I feel like you are trying to blame the victim.
>Maybe that's just your reaction to my forcing the issue after months of
>frustration. 

I am not trying to blame you for the problem.  I am trying to help you.
But I can not help you unless you admit that you made a mistake by relying
too much on private email, and by using inappropriate tactics to try to
force the issue.

Yes, you were frustrated, but no one knew that you were frustrated, and
thus no one was able to help you.  To fix that problem, you need to send
messages to public mailing lists instead of sending only private mail, or
else put an audit-trail in the PR.  That way, everyone is aware of what is
going on.

Also, you conveniently left out the fact that your original mesasge made
veiled accusations against unnamed targets with no supporting evidence.
I don't like to see people's reputations unfairly tarnished, particularly
when the accusations were ambiguous enough that people might think that I
personally was at fault.  This action was definitely not supportable by
the public evidence.

But to get back to the real problem here, the lack of patch review for
Zoltan's patch... Bernd is already reviewing it as we speak, and I have
already stated that I am willing to review the patch, so I will follow up
if Bernd doesn't finish resolving the issue.

As for the larger issue of patch review in general, Joe Buck has started
a new thread to discuss it.  A thread that is largely free of invective.
I hope it stays that way, and I hope that some good ideas come out of it.

Jim

Re: Loop unroll fixes

[David Edelsohn]

	Zoltan repeatedly posted messages at various intervals asking
about the patch.  My private communication only followed up after each of
his repeated public requests.

	I do not accept your statement that the patch and its lack of
attention was secret.  I feel like you are trying to blame the victim.
Maybe that's just your reaction to my forcing the issue after months of
frustration. 

David

Re: Loop unroll fixes

[Jim Wilson]

>I did not voice the ultimatum only based on the actions and
>messages archived.  I made the statement after months of private action
>and discussions.

I will accept your statement that you sent private mail that was ignored.
I will not accept your statement that this justifies your action.  There
is no public evidence that a patch was ignored.  Therefore, there is no public
evidence to justify your action.  I will not accept secret communications
as justifcation for your action, hence your action was unwarranted.

In my opinion, your over reliance on private mail was a mistake here.
First of all, you create a single point of failure.  If only one person
is given a chance to review a patch, then it is much less likely to be
reviewed than a patch sent to a mailing list.  That one person might be
busy, or might be on vacation.  Also, it has the effect of hiding the
problem.  You knew that the patch was overdue for a review, but the gcc
developer community did not know, because the patch was not discussed in
public.  If the patch had been discussed in public, other people would
have volunteered to help, and the patch would not have been ignored for
two months.

>I have reviewed the patch.  I and others have tested the patch.

That is good to know.  However, it should have been stated publicly before
you took drastic action.  As far as I know, it was not.  There was only
a statement from the contributor claiming that the patch had been tested.

>	GCC SC members and other developers with GCC commit privileges
>need to take action when the maintainers do not respond to repeated,
>polite prompting.

I can accept that.  But I can not accept use of secret communications
as justification for such actions.  There is too much at stake here.
The only way we can ensure that such action have a valid justification
is by relying on public communications.  That way, everyone knows what
we are doing, and why.  Also, we are much less likely to make mistakes
this way, as people get a chance to correct errors in arguments before
decisions are made.

>	Your vindictive response of further delaying the patch does not
>help GCC or anyone.  You yourself were one of the most outspoken critics
>of the previous GCC development delays and inability to get patches into
>the FSF sources.  How ironic that you do not see yourself falling into the
>same trap and creating the same situation.

My dictionary defines vindicitive as "disposed or inclined to revenge".
I offered to help you get the patch into the FSF sources.  I fail to see
how that constitutes an act of revenge.

I did qualify the form of my assistance, saying that there would be a one
week delay.  This is not intended to be a revengeful act.  It is intended to
be a disciplinary act.  As I see it, you did two things wrong
1) You made a major decision based on secret communications.
2) You made veiled accusations that were easily misinterpreted.
I do not wish to see these things continue, so I choose an action that I
hoped would discourage you from repeating these things in the future, while
at the same time offering help to resolve your problem.

>You yourself were one of the most outspoken critics
>of the previous GCC development delays and inability to get patches into
>the FSF sources.

I was vocal about patch review problems before the EGCS project started.
Things are much better now.  However, no matter how much we improve the
process, there will always be problems.  Not everyone that wants to contribute
to gcc has sufficient technical skill to write good patches.  Not everyone
that wants to contribute will have a personality that meshes with other
gcc developers.  Not every patch contributed is a good idea.  There will
always be patches that slip through the cracks no matter how good the process
is.  Overall, I think we are doing a reasonable job.  People who are
persistent, who do consistently good work, and who show a willingness to
finish what they start, generally are able to get their patches in.  People
who are casual contributors will likely have problems, but that is a tough
problem to solve without endangering the stability of the sources.  At least
we have the GNATS database now, which gives us a place to put patches where
they won't be lost.  I don't know how to improve on that without hiring
someone to do the task, and we don't have money for that.

>How ironic that you do not see yourself falling into the
>same trap and creating the same situation.

I fail to see how I am adding to the problem.  After all, I did offer my help
to review the patch.  I could not offer my help earlier, because I was not
privy your secret communications.

Jim

Re: Loop unroll fixes

[Jim Wilson]

>No, I'd rather believe he's trying to just bully people into reviewing
>a patch.
>Which is good, in reality.

I can understand that you are probably responding in frustration, perhaps from
personal experience with problems with patch reviews, but I still don't think
that justifies a lack of civility in this case.

If there was evidence in the public record that a maintainer was unresponsive,
then yes, bullying would be appropriate.  But bullying should not be the
first approach, it should be the last resort.  There is no public evidence
to indicate that an maintainer was unresponsive.  And there is public evidence
indicating that bullying was the first approach used.

>It shouldn't take months to review patches.

Ideally, yes, but we get so many patches that no matter how good the process
is, there will always be some patches that are delayed.  I believe the
majority of patches get a response in a reasonable amount of time.  When
someone points out unreviewed patches, they usually get a response in a
reasonable amount of time.  Most patches that are delayed after that are
controversial and/or potentially disruptive.  That is not unreasonable.
I agree that the current process is a pain to work with, but overall I think
it is working reasonably well.

In this particular case, I presented evidence showing that there was one
month delay because of miscommunication, and one more month of delay because
of a conflict with the gcc 3.0.1 release process.  Thus the two month delay
in this particular case, though unfortunate, is perfectly understandable,
and does not indicate any serious flaws with the current system.

>This is the wrong approach.
>Your job, as a maintainer, is to review patches.
>It's not some "honor", or exalted position.
>You are supposed to review every patch, that comes into your area, in
>a reasonable amount of time.  Months is not reasonable.

I presented ample evidence in my last message to demonstrate that there is
no basis for this accusation in this particular case.  Making this claim
is offensive, and couterproductive.  I did volunteer to review this patch,
and now my thanks for my trouble is getting a flame from you.  If volunteers
get flamed for trying to help, then they will be less willing to help in the
future, and we will only have more trouble with patch reviews.

>Deferring action for a week just looks like you are trying to have a
>fiefdom of your own.

Another offensive comment.  My approach is easy to understand, it is a simple
application of negative reinforcement.  If someone uses threats against me,
then I will delay cooperating with them.  The more I am threated, the more
I will delay.  Eventually, I expect people will learn that threats are
counterproductive, and stop using them.  I am willing to accept the risk
that this approach will occasionally backfire.  I will not accept the claim
that this approach is wrong.  After all, I am a volunteer, and I reserve the
right to decide what I do with the time that I have volunteered to spend on
gcc.

Jim

Re: Loop unroll fixes

[David Edelsohn]

>>>>> Bernd Schmidt writes:

Bernd> Then please describe the nature of the bug and how the patch fixes it.

Bernd> I would have liked a "walkthrough" of the problem; some detailed information
Bernd> on what transformation is being done, which part of the compiler does it and
Bernd> why, what's wrong with it etc.  I was hoping you could
Bernd> provide it since you reviewed the patch?

	I just sent three messages to gcc-patches breaking out the three
independent pieces of Zoltan's original patch and explaining each one
using my own description of what it fixes and how it fixes the incorrect
code generation.

	I don't know if this is sufficiently thorough.  My explanation is
more detailed and specific for each hunk.  The separate messages should
clarify which parts are independent.

Thanks, David

Re: Loop unroll fixes

[Joseph S. Myers]

On Fri, 14 Sep 2001, Daniel Berlin wrote:

> Your job, as a maintainer, is to review patches.

As a maintainer, this is not the case; every patch can be reviewed by at
least the twelve people with global write privileges and there is no
particular requirement on any one person to review a particular patch.

As a developer (of patches touching other areas of the compiler), we need
a patch tracking system to avoid patches getting lost.

As both, even trivial patches can fail to attract the necessary attention,
especially if they do need a global write maintainer or multiple
maintainers to look at them.  E.g.,
<URL: http://gcc.gnu.org/ml/gcc-patches/2001-06/msg01412.html > (a simple
patch to help i18n) still awaits review of the one-line patch to exgettext
(the C front end part is OK).  A lot of patches are, like this, largely
within one maintainer's competence but for trivial reasons involve changes
elsewhere requiring review from someone else as well.

-- 
Joseph S. Myers
jsm28@cam.ac.uk

Re: Loop unroll fixes

[David Edelsohn]

	If the GCC developers with the experience and privileges to review
patches don't have enough time to review all of the patches, then we
explicitly should tell the GCC developer community that.  Implying that
GCC development can handle the volume of patches, but letting most patches
fall through the cracks helps no one.  We need to attract more GCC
developers to whom responsibility can be spread, not discourage the very
developers who could become those maintainers and reviewers.

	We also have to accept that fixing the problem will require
modifications to GCC development policy in order to balance responsiveness
to developers with stability of the compiler.  We already have started to
address that with the new rules for checking in and reverting patches.

David

	P.S. I am sorry that my original message upset some people, but I
think it is good if it can lead to this type of open discussion about the
problems in GCC development and how to improve the situation.

Re: Loop unroll fixes

[Bernd Schmidt]

On Fri, 14 Sep 2001, David Edelsohn wrote:

> 	Another GCC developer did respond to my initial request to review
> the patch.  He asked that the patch not be applied until he had reviewed
> it, although he did not have any specific objection when asked.  That
> person has not followed up after initially blocking the patch.

That probably was me, although I recall the details slightly differently.
Of course I can't give a specific objection if I haven't even looked at
the problem.  None of Zoltan's messages contained a good description of
the problem, only testcases - so any potential reviewer would have to
debug the problem and figure out what's going wrong.

> 	I have reviewed the patch.

Then please describe the nature of the bug and how the patch fixes it.

> I have received numerous emails from developers
> extremely frustrated with patches being ignored, not just the complaints
> posted on the GCC mailinglists.

So what do you suggest we do?  Apply patches without understanding them?
I doubt that's going to lead to better quality code.  I think that even
now, patches aren't reviewed extensively enough - a lot of the breakage
we regularly have could be avoided if more time was spent on review.  But
more time is not available.


Bernd

Re: Loop unroll fixes

[David Edelsohn]

	Another GCC developer did respond to my initial request to review
the patch.  He asked that the patch not be applied until he had reviewed
it, although he did not have any specific objection when asked.  That
person has not followed up after initially blocking the patch.

	I and others have made numerous private requests to GCC
maintainers throughout the entire summer.  Those messages have not been
archived.  None of those messages elicited a response, although other
messages unrelated to this issue did.  A response of "I'm too busy" or "I
will look at it" or "I don't have a problem with the patch" or any
confirmation that the person had received the request would have been
acceptable.

	I did not voice the ultimatum only based on the actions and
messages archived.  I made the statement after months of private action
and discussions.

	I have reviewed the patch.  I and others have tested the patch.

	GCC SC members and other developers with GCC commit privileges
need to take action when the maintainers do not respond to repeated,
polite prompting.  I have received numerous emails from developers
extremely frustrated with patches being ignored, not just the complaints
posted on the GCC mailinglists.  I guess that my activism on this issue is
eliciting more messages than other GCC SC members receive.  The GCC SC and
the FSF are a voice for developers who feel disenfranchised.

	GCC evolution toward a more open development model has recruited
many new developers and drawn an influx of patches.  Hoarding of authority
amongst developers who are too busy to handle the load only returns us to
the discouraging development model GCC previously used.

	Your vindictive response of further delaying the patch does not
help GCC or anyone.  You yourself were one of the most outspoken critics
of the previous GCC development delays and inability to get patches into
the FSF sources.  How ironic that you do not see yourself falling into the
same trap and creating the same situation.

David

Re: Loop unroll fixes

[Daniel Berlin]

Jim Wilson <wilson@cygnus.com> writes:

> The gcc development process does not work that way.  Sometimes it does take
> months before someone has time to review a patch.
>  You can't check in a
> patch just because someone didn't review it fast enough to make you happy.
>
> I also object on the grounds that you have done this twice now.  Once in
> PR 3384 as described above, and once in todays mail.  You are trying to
> bully people into reviewing a patch that furthers your own personal
> agenda.
No, I'd rather believe he's trying to just bully people into reviewing
a patch.
Which is good, in reality.
It shouldn't take months to review patches.
If it takes bullying to make review of patches happen, so be it.
If it takes adding more people who have blanket write privs, or
whatever, so be it.
The idea that a patch *should* take months to review is just plain
absurd, unless it's rewriting half of gcc.

>
> No, I didn't forget about the patch.  I am willing to review it, but in
> protest of the heavy handed tactics being used, and the complexity of the
> problem, I will defer taking action for one week.
This is the wrong approach.
Your job, as a maintainer, is to review patches.
It's not some "honor", or exalted position.
You are supposed to review every patch, that comes into your area, in
a reasonable amount of time.  Months is not reasonable.
If you can't do this, don't be a maintainer.
Deferring action for a week just looks like you are trying to have a
fiefdom of your own.

>
> Jim

-- 
"My girlfriend does her nails with white-out.  When she's asleep,
I go over there and write misspelled words on them.
"-Steven Wright

Re: Loop unroll fixes

[Jim Wilson]

In article < 200109140157.VAA25844@makai.watson.ibm.com > you write:
>	The maintainer of the part of the compiler affected by the patch
>has not responded to repeated requests for his feedback.  One other
>developer has asked for the approval of the patch to be delayed until he
>reviewed it, but has not done so.

The subject line says "Loop unroll fixes", and the MAINTAINERS file says
that I am the loop unroll maintainer, so this might lead someone to think
that I have been negligent.  I haven't.

I only recall getting two direct messaages on this topic.  One said nothing
interesting:
http://gcc.gnu.org/ml/gcc-bugs/2001-07/msg00775.html

The other apparently wasn't sent to the lists, but it is archived in
PR 3384.  I quote:
	If Michael Hayes does not review the patch by Tuesday and
	Jim Wilson does not object, Mark Mitchell and I have agreed
	to approve the patch for the mainline. Assuming no problems
	appear, we will approve the patch for the gcc-3.0 branch
	by the time Mark creates the tarball for testing
Any reasonable person named Jim Wilson seeing this would assume that
a) a response is optional
b) whether or not a response is sent, someone else has accepted responsibility
   for resolving the problem

I see no other evidence on the gcc web site that other messsages were sent to
me.

Perhaps you were referring to Michael Hayes, but I see no evidence that Michael
Hayes is guilty of negligence either.  In fact, he did give a partial approval
for one of the patches in one of his messages.
http://gcc.gnu.org/ml/gcc-bugs/2001-07/msg01101.html
I quote:
	The doloop part of this patch is OK.

Michael Hayes recieved and replied to a number of messages on this thread.
I see no evidence on the gcc web site that there were repeated requests for
feedback that were ignored.

There is other relevant evidence from other peoples mail mesasges.
There was a message from Geoff Keating saying he would put it on his queue.
http://gcc.gnu.org/ml/gcc-bugs/2001-07/msg00769.html

There was also a message from Bernd Schmidt claiming to have checked in a
patch that fixed the problem.
http://gcc.gnu.org/ml/gcc-patches/2001-07/msg01056.html

So we have a case where at least 6 different gcc maintainers looked at the
patch, 3 of them making some claim to have contributed to a fix.  It is no
surprise then that something might have gone wrong.  Everyone assumed someone
else was going to fix it, so no one did.

There was a brief dialog about the problem in August, but it was requested
to resubmit after gcc 3.0.1.
http://gcc.gnu.org/ml/gcc/2001-08/msg00845.html
http://gcc.gnu.org/ml/gcc/2001-08/msg00848.html
http://gcc.gnu.org/ml/gcc/2001-08/msg00908.html

The problem was then dormant again until resubmitted today.
http://gcc.gnu.org/ml/gcc/2001-09/msg00526.html
At this point, you claim that because others have been negligent, you have
the authority to check in the patch even though it hasn't been approved.
There is however no evidence that anyone has been negligent, and much
evidence that there has been miscommunication.

>	If there is no specific, documented, technical objection to this
>patch within one week, it will be approved.  Any problems will be handled
>by the GCC policy for patches which have been committed.  If anyone has
>concerns about this patch, review it now.

I object.  My objection is detailed above.  It is specific, and well
documented.

I also object on the grounds that you are overstepping your authority.
I know of no part of the gcc development process that gives you the
authority to check in the patch.  If there is one, please state it.

I also object to the way that you are setting short deadlines, and threatening
to check in the patch if no one reviews it at your own convenience.
The gcc development process does not work that way.  Sometimes it does take
months before someone has time to review a patch.  You can't check in a
patch just because someone didn't review it fast enough to make you happy.

I also object on the grounds that you have done this twice now.  Once in
PR 3384 as described above, and once in todays mail.  You are trying to
bully people into reviewing a patch that furthers your own personal agenda.
I would expect better of someone who is a GCC Steering Committee member.

I also object on the grounds that you have not provided any technical evidence
to support the patch.  I see no evidence that you have tested the patch.
I see no evidence that you have reviewed the patch.  I see no evidence
that you have said anything good about the patch.  All I see of substance
are the two requests trying to bully people into reviewing the patch for you.

No, I didn't forget about the patch.  I am willing to review it, but in
protest of the heavy handed tactics being used, and the complexity of the
problem, I will defer taking action for one week.

Jim

Re: Loop unroll fixes

[David Edelsohn]

	The maintainer of the part of the compiler affected by the patch
has not responded to repeated requests for his feedback.  One other
developer has asked for the approval of the patch to be delayed until he
reviewed it, but has not done so.

	If there is no specific, documented, technical objection to this
patch within one week, it will be approved.  Any problems will be handled
by the GCC policy for patches which have been committed.  If anyone has
concerns about this patch, review it now.

Thanks, David

Loop unroll fixes

[Zoltan Hidvegi]

Sorry to bug you all with that again, but I wonder if the fix for PR
3384 could be applied to the 3.0 branch and the mainline (on the
mainline the doloop.c patch conflicts with Bernd Smith's change to fix
a subset of the bugs fixed by my patches that also generates a slighly
less efficient code).  See PR 3384, and threads
http://gcc.gnu.org/ml/gcc-patches/2001-07/msg01080.html
http://gcc.gnu.org/ml/gcc-bugs/2001-07/msg00750.html

I think this is a bad regression from 2.95.2.  -funroll-loops is a
powerful and widely used optimization.  GNATS already have a high
priority assigned to this bug, please, someone look at it and either
check it in or do some alrernative fix.  Bootstrapped and passed the
regressions on powerpc-ibm-aix4.3.3 and on i686-pc-linux-gnu, at least
it did some time back just before 3.0.1 was released, but there has
been no change in the files involved since than.

Thanks very much,

Zoli