state of 3.2.1-pre: how far from release?

state of 3.2.1-pre: how far from release?

[Joe Buck]

We currently have 16 high-priority PRs against the 3.2 branch
(that is, excluding those PRs marked [mainline regression]).

There is no hope that the 3.2.1 release will be regression-free in the
way that we are currently defining regressions: that *some* preceding
release compiled the code correctly.  Such a criterion would require that
3.2.1 fix every issue introduced into GCC since 3.0.  Clearly we're not
going to do that, and will wind up needing 3.2.2 (unless we decide to
go straight to 3.3, which might not be a great idea considering the
15 additional regressions in the trunk).

I think it's reasonable to expect that 3.2.1 will not be worse than
3.2, and we have one regression against 3.2:

c++/8338	3.2 works, branch gives segfault in cc1plus
(the synopsis is no longer correct, it is an ICE not an infinite loop).

Still, this is for illegal code.

The following testcases have been broken in all releases from 3.0 onward:

c++/8021 c++/8372 c++/8453

I propose that we punt on those for 3.2.1.

The following testcases have been broken from 3.1 onward, and are also
broken on the branch, but are fixed (or at least don't occur) in the CVS
trunk:

c/5351 c++/8117 c++/8205

I think we should decide whether backporting is worthwhile, or if we
should just say "This issue will be fixed in 3.3".  Nothing here should
block 3.2.1.

The following testcases are broken for gcc 3.1 and later, and are still
broken in the branch and the trunk:

libstdc++/6746 c++/8036 c++/8116 c/8439 c++/8442

I don't think that any of these should block 3.2.1.

The following two PRs are for m68-elf/rtems:

target/8343
	This was narrowed down to a change Honza made, and the log says
	he's looking at it.  Any updates?
target/8314
	Here Joel was testing a patch and reassigned to Jeff Law.
	Any updates here?

Given that these are in progress, clearly fixes could go in, but again
nothing should block 3.2.1.

Finally, there are two that I am unclear about:

bootstrap/8362
	This is PowerPC specific and appears to be a blocker.

bootstrap/8146
	We still don't know what the issue is here (with building
	2.95.3).  I'd like to understand it.  But even if we do
	nothing, 3.2 has the same issue as the 3.2.1 branch, at
	least it's no worse.

My conclusion, IMHO:

3.2.1 has a vast number of bug fixes over 3.2.  If we can get 8362
fixed, I'd say ship the sucker, it is in the best interest of the
users to get the fixes out there (especially all the fixes to the
x86 backend).  The other bugs are not that important.

Re: state of 3.2.1-pre: how far from release?

[David Edelsohn]

>>>>> Joe Buck writes:

Joe> bootstrap/8362
Joe> This is PowerPC specific and appears to be a blocker.

Joe> 3.2.1 has a vast number of bug fixes over 3.2.  If we can get 8362
Joe> fixed, I'd say ship the sucker, it is in the best interest of the
Joe> users to get the fixes out there (especially all the fixes to the
Joe> x86 backend).  The other bugs are not that important.

	I can guard the source code in abi_check.cc which elicits the ICE
so that the compiler will bootstrap.  This will not fix the bug.  The
testcase takes a very long time to compile when running the compiler in
the debugger, so it is difficult to debug.  The rest of libstdc++-v3
compiles, so this bug is not extremely common but it is not complicated
either.

	I believe the testcase is uncovering a serious, latent bug in the
common part of the compiler.  GCC has plenty of other "how did that ever
work" bugs, so GCC 3.2.1 could be released with this bug too.

	Without help from GCC experts who understand the reload pass, this
will take a while to debug and fix.

David

Re: state of 3.2.1-pre: how far from release?

[Geoff Keating]

David Edelsohn <dje@watson.ibm.com> writes:

> >>>>> Joe Buck writes:
> 
> Joe> bootstrap/8362
> Joe> This is PowerPC specific and appears to be a blocker.
> 
> Joe> 3.2.1 has a vast number of bug fixes over 3.2.  If we can get 8362
> Joe> fixed, I'd say ship the sucker, it is in the best interest of the
> Joe> users to get the fixes out there (especially all the fixes to the
> Joe> x86 backend).  The other bugs are not that important.
> 
> 	I can guard the source code in abi_check.cc which elicits the ICE
> so that the compiler will bootstrap.  This will not fix the bug.  The
> testcase takes a very long time to compile when running the compiler in
> the debugger, so it is difficult to debug.  The rest of libstdc++-v3
> compiles, so this bug is not extremely common but it is not complicated
> either.
> 
> 	I believe the testcase is uncovering a serious, latent bug in the
> common part of the compiler.  GCC has plenty of other "how did that ever
> work" bugs, so GCC 3.2.1 could be released with this bug too.
> 
> 	Without help from GCC experts who understand the reload pass, this
> will take a while to debug and fix.

In the interest of getting this release shipped, I would recommend
disabling the load_multiple pattern on the branch.  Whatever fix is
finally produced, it will touch reload/reg-alloc and so probably won't
be suitable for the branch at this point before a release anyway.

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Re: state of 3.2.1-pre: how far from release?

[Joe Buck]

I wrote:
> Joe> bootstrap/8362
> Joe> This is PowerPC specific and appears to be a blocker.
> 
> Joe> 3.2.1 has a vast number of bug fixes over 3.2.  If we can get 8362
> Joe> fixed, I'd say ship the sucker, it is in the best interest of the
> Joe> users to get the fixes out there (especially all the fixes to the
> Joe> x86 backend).  The other bugs are not that important.

David Edelsohn writes:
> 	I can guard the source code in abi_check.cc which elicits the ICE
> so that the compiler will bootstrap.  This will not fix the bug.  The
> testcase takes a very long time to compile when running the compiler in
> the debugger, so it is difficult to debug.  The rest of libstdc++-v3
> compiles, so this bug is not extremely common but it is not complicated
> either.

Perhaps that's the best thing for now.  I would be satisfied with a
workaround for 3.2.1.

> 	Without help from GCC experts who understand the reload pass, this
> will take a while to debug and fix.

Don't we plan to redo/replace reload for 3.4?  Or am I confused?

In any case: Mark, what's your thinking?  Are there other bugs that you
consider blockers?

In any case, if (or rather, when) we release 3.2.1 with still-open
regressions, I'll add a description of the regressions to the release
notes.

Re: state of 3.2.1-pre: how far from release?

[Mark Mitchell]

> Perhaps that's the best thing for now.  I would be satisfied with a
> workaround for 3.2.1.


>> 	Without help from GCC experts who understand the reload pass, this
>> will take a while to debug and fix.
>
> Don't we plan to redo/replace reload for 3.4?  Or am I confused?
>
> In any case: Mark, what's your thinking?  Are there other bugs that you
> consider blockers?

That's a good question, and one I was just looking at.  I am concerned
about 8453 which is a silent code-gen failure in C++, and 8467 which
is a silent code-gen failure with sibcalls, and 5351 which is a
silent code-gen failure with structure passing.

I am also concerned about 6746, which is a libstdc++ problem; I mentioned
it to Benjamin some time back, but there has been no action in GNATS.

I am also concerned about 8146, the bootstrap failure with GCC 2.95.3.  Do
we yet have information about what is causing the problem?

These are all problems that could cause people real problems.

I don't see that we have many options; people are more interested in
working on BIB than 3.2.1, I have cajoled about as much as I am able,
and I have no other tools at my disposal.

My preference would be to see the load multiple/store multiple
instructions that are causing the problem disabled.  Better a slow
compiler than a broken one, since David says the problem can come up
relatively often.

If the RS6000 maintainers do not want to do that, then David's
work-around for abi_check.cc is OK.

I would like the RS6000 maintainers to decide one way or the other,
and check in one or the other approach ASAP.

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

Re: state of 3.2.1-pre: how far from release?

[Joe Buck]

> > In any case: Mark, what's your thinking?  Are there other bugs that you
> > consider blockers?
> 
> That's a good question, and one I was just looking at.  I am concerned
> about 8453 which is a silent code-gen failure in C++, and 8467 which
> is a silent code-gen failure with sibcalls, and 5351 which is a
> silent code-gen failure with structure passing.

I didn't have 8467 in my list, it has just been raised to "high".
Could it have the same underlying causes as 8362?

Also, I have just verified that a simple patch from HJ fixes 5351 (see his
earlier message of today on gcc).  Also, 7591 is a duplicate of 5351, so
at least two folks ran into it.

> I am also concerned about 8146, the bootstrap failure with GCC 2.95.3.  Do
> we yet have information about what is causing the problem?

This bug also appears to be an i586/i686 difference, suggesting that it
might be the same issue as 5351.  (that is, folks report that if gcc 3.2
is told to tune for i586 the bug does not occur).  I plan to test as well
to see if HJ's patch addresses that one.  In any case, I'm asking you
and/or RTH to look at HJ's patch again.  Evidently 5351 does not occur on
the trunk, so maybe even if you're not thrilled about the concept behind
HJ's patch it might be well worth doing on the branch assuming no new
regressions (I'll run tests on that score soon).

> I don't see that we have many options; people are more interested in
> working on BIB than 3.2.1, I have cajoled about as much as I am able,
> and I have no other tools at my disposal.

You have one tool at your disposal: you could set a deadline.  As I've
said, even though 3.2.1 still contains nasties it appears to have far
fewer nasties than 3.2.

> If the RS6000 maintainers do not want to do that, then David's
> work-around for abi_check.cc is OK.
> 
> I would like the RS6000 maintainers to decide one way or the other,
> and check in one or the other approach ASAP.

Agreed.

Re: state of 3.2.1-pre: how far from release?

[David Edelsohn]

>>>>> Mark Mitchell writes:

Mark> My preference would be to see the load multiple/store multiple
Mark> instructions that are causing the problem disabled.  Better a slow
Mark> compiler than a broken one, since David says the problem can come up
Mark> relatively often.

Mark> If the RS6000 maintainers do not want to do that, then David's
Mark> work-around for abi_check.cc is OK.

	The failure is not complicated, but does not appear to occur
often, so disabling the pattern does not seem necessary.  We can consider
this more for GCC 3.3.

David

Re: state of 3.2.1-pre: how far from release?

[Mark Mitchell]

--On Wednesday, November 06, 2002 07:20:03 PM -0500 David Edelsohn 
<dje@watson.ibm.com> wrote:

>>>>>> Mark Mitchell writes:
>
> Mark> My preference would be to see the load multiple/store multiple
> Mark> instructions that are causing the problem disabled.  Better a slow
> Mark> compiler than a broken one, since David says the problem can come up
> Mark> relatively often.
>
> Mark> If the RS6000 maintainers do not want to do that, then David's
> Mark> work-around for abi_check.cc is OK.
>
> 	The failure is not complicated, but does not appear to occur
> often, so disabling the pattern does not seem necessary.  We can consider
> this more for GCC 3.3.

I'm happy to trust your judgement; please check in a change, and mark
the PR as a mainline regression, since we'll not be worrying about it
on the branch.

Thanks,

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

Re: state of 3.2.1-pre: how far from release?

[Benjamin Kosnik]

>I am also concerned about 6746, which is a libstdc++ problem; I mentioned
>it to Benjamin some time back, but there has been no action in GNATS.

I'm back and working on this now.

-benjamin

Re: state of 3.2.1-pre: how far from release?

[David Edelsohn]

	Let me explain my current understanding of the bug and maybe
someone can provide some more insight.  The bug occurs on the GCC 3.2
branch, but not on the trunk.

	On the GCC 3.2 branch, reload_as_needed() is entered with the
instruction still containing pseudos.  eliminate_regs_in_insn()
substitutes a hard reg for the first use of the pseudo, but leaves the
others untouched.  eliminate_regs_in_insn() recognizes the instruction and
recog_data.n_operands is 3, so it only loops over three locations.  On the
trunk, the instruction already has hard regs substituted when
reload_as_needed() is called.

	Are pseudos suppose to be shared so that one substitution replaces
all instances?  Shared within an instruction?  The parallel is generated
using:

  for (i = 0; i < count; i++)
    XVECEXP (operands[3], 0, i)
      = gen_rtx_SET (VOIDmode, gen_rtx_REG (SImode, regno + i),
                     adjust_address (op1, SImode, i * 4));

and adjust_address() calls copy_rtx() on the addr in the memref.  Maybe
the bug is calling adjust_address() to generate a sequence of memory
addresses in a single instruction.

Thanks, David

Re: state of 3.2.1-pre: how far from release?

[Michael Matz]

Hi,

On Tue, 5 Nov 2002, David Edelsohn wrote:

> 	On the GCC 3.2 branch, reload_as_needed() is entered with the
> instruction still containing pseudos.

This can't happen if there weren't bugs before reload:

> eliminate_regs_in_insn() substitutes a hard reg for the first use of
> the pseudo, but leaves the others untouched.
> eliminate_regs_in_insn() recognizes the instruction and
> recog_data.n_operands is 3, so it only loops over three locations.
> On the trunk, the instruction already has hard regs substituted when
> reload_as_needed() is called.
>
> 	Are pseudos suppose to be shared so that one substitution replaces
> all instances?

Yes, pseudo reg RTL _has_ to be shared.  There needs to be exactly one
instance of (reg:MODE x) if x >= FIRST_PSEUDO_REGISTER, and it is placed
into regno_reg_rtx[x].  If there is a need to use a pseudo in a different
mode than MODE, you have to use subreg's.

> Shared within an instruction?  The parallel is generated
> using:
>
>   for (i = 0; i < count; i++)
>     XVECEXP (operands[3], 0, i)
>       = gen_rtx_SET (VOIDmode, gen_rtx_REG (SImode, regno + i),
>                      adjust_address (op1, SImode, i * 4));

Which rtx do contain the pseudos here?  regno is a hard reg, right?  So
does op1 contain pseudos?

> and adjust_address() calls copy_rtx() on the addr in the memref.  Maybe

copy_rtx() applied to a pseudo reg rtx returns it, so copy_rtx() never
(should) create non-shared pseudos.  The only way I currently remember is
to use gen_rtx_REG() on a pseudo reg number directly.

> the bug is calling adjust_address() to generate a sequence of memory
> addresses in a single instruction.


Ciao,
Michael.

Re: state of 3.2.1-pre: how far from release?

[David Edelsohn]

>>>>> Michael Matz writes:

Michael> Which rtx do contain the pseudos here?  regno is a hard reg, right?  So
Michael> does op1 contain pseudos?

	A larger fragment of the code generating the parallel is:

  operands[3] = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (count));
  op1 = replace_equiv_address (operands[1],
                               force_reg (SImode, XEXP (operands[1], 0)));

  for (i = 0; i < count; i++)
    XVECEXP (operands[3], 0, i)
      = gen_rtx_SET (VOIDmode, gen_rtx_REG (SImode, regno + i),
                     adjust_address (op1, SImode, i * 4));

op1 contains the memref forced into a reg.  This produces RTL like:

(insn 1842 11909 11910 (set (reg/f:SI 929)
        (plus:SI (reg/f:SI 31 r31)
            (const_int 520 [0x208]))) 36 {*addsi3_internal1} (nil)
    (expr_list:REG_EQUIV (plus:SI (reg/f:SI 31 r31)
            (const_int 520 [0x208]))
        (nil)))

(insn 1856 1853 16458 (parallel[ 
            (set (reg:SI 4 r4)
                (mem/s:SI (reg/f:SI 929) [0 S4 A32]))
            (set (reg:SI 5 r5)
                (mem/s:SI (plus:SI (reg/f:SI 929)
                        (const_int 4 [0x4])) [0 S4 A32]))
            (set (reg:SI 6 r6)
                (mem/s:SI (plus:SI (reg/f:SI 929)
                        (const_int 8 [0x8])) [0 S4 A32]))
            (set (reg:SI 7 r7)
                (mem/s:SI (plus:SI (reg/f:SI 929)
                        (const_int 12 [0xc])) [0 S4 A32]))
        ] ) 322 {*rs6000.md:8746} (insn_list:REG_DEP_OUTPUT 1843 (insn_list:REG_DEP_OUTPUT 1846 (insn_list 1842 (insn_list:REG_DEP_ANTI 1849 (nil)))))
    (nil))

eliminate_regs_in_insn() modification of the pseudo in the first
expression does not affect the pseudo in the other expressions.

David

Re: state of 3.2.1-pre: how far from release?

[Daniel Jacobowitz]

On Wed, Nov 06, 2002 at 10:32:38AM -0500, David Edelsohn wrote:
> >>>>> Michael Matz writes:
> 
> Michael> Which rtx do contain the pseudos here?  regno is a hard reg, right?  So
> Michael> does op1 contain pseudos?
> 
> 	A larger fragment of the code generating the parallel is:
> 
>   operands[3] = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (count));
>   op1 = replace_equiv_address (operands[1],
>                                force_reg (SImode, XEXP (operands[1], 0)));
> 
>   for (i = 0; i < count; i++)
>     XVECEXP (operands[3], 0, i)
>       = gen_rtx_SET (VOIDmode, gen_rtx_REG (SImode, regno + i),
>                      adjust_address (op1, SImode, i * 4));
> 
> op1 contains the memref forced into a reg.  This produces RTL like:
> 
> (insn 1842 11909 11910 (set (reg/f:SI 929)
>         (plus:SI (reg/f:SI 31 r31)
>             (const_int 520 [0x208]))) 36 {*addsi3_internal1} (nil)
>     (expr_list:REG_EQUIV (plus:SI (reg/f:SI 31 r31)
>             (const_int 520 [0x208]))
>         (nil)))
> 
> (insn 1856 1853 16458 (parallel[ 
>             (set (reg:SI 4 r4)
>                 (mem/s:SI (reg/f:SI 929) [0 S4 A32]))
>             (set (reg:SI 5 r5)
>                 (mem/s:SI (plus:SI (reg/f:SI 929)
>                         (const_int 4 [0x4])) [0 S4 A32]))
>             (set (reg:SI 6 r6)
>                 (mem/s:SI (plus:SI (reg/f:SI 929)
>                         (const_int 8 [0x8])) [0 S4 A32]))
>             (set (reg:SI 7 r7)
>                 (mem/s:SI (plus:SI (reg/f:SI 929)
>                         (const_int 12 [0xc])) [0 S4 A32]))
>         ] ) 322 {*rs6000.md:8746} (insn_list:REG_DEP_OUTPUT 1843 (insn_list:REG_DEP_OUTPUT 1846 (insn_list 1842 (insn_list:REG_DEP_ANTI 1849 (nil)))))
>     (nil))

At this point are the four copies of (reg/f:SI 929) still shared?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: state of 3.2.1-pre: how far from release?

[Michael Matz]

Hi,

On Wed, 6 Nov 2002, David Edelsohn wrote:

> 	A larger fragment of the code generating the parallel is:
>
>   operands[3] = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (count));
>   op1 = replace_equiv_address (operands[1],
>                                force_reg (SImode, XEXP (operands[1], 0)));
>
>   for (i = 0; i < count; i++)
>     XVECEXP (operands[3], 0, i)
>       = gen_rtx_SET (VOIDmode, gen_rtx_REG (SImode, regno + i),
>                      adjust_address (op1, SImode, i * 4));
>
> op1 contains the memref forced into a reg.  This produces RTL like:
>
> (insn 1842 11909 11910 (set (reg/f:SI 929)
>         (plus:SI (reg/f:SI 31 r31)
>             (const_int 520 [0x208]))) 36 {*addsi3_internal1} (nil)
>     (expr_list:REG_EQUIV (plus:SI (reg/f:SI 31 r31)
>             (const_int 520 [0x208]))
>         (nil)))
>
> (insn 1856 1853 16458 (parallel[
>             (set (reg:SI 4 r4)
>                 (mem/s:SI (reg/f:SI 929) [0 S4 A32]))
>             (set (reg:SI 5 r5)
>                 (mem/s:SI (plus:SI (reg/f:SI 929)
>                         (const_int 4 [0x4])) [0 S4 A32]))
>             (set (reg:SI 6 r6)
>                 (mem/s:SI (plus:SI (reg/f:SI 929)
>                         (const_int 8 [0x8])) [0 S4 A32]))
>             (set (reg:SI 7 r7)
>                 (mem/s:SI (plus:SI (reg/f:SI 929)
>                         (const_int 12 [0xc])) [0 S4 A32]))
>         ] ) 322 {*rs6000.md:8746} (insn_list:REG_DEP_OUTPUT 1843 (insn_list:REG_DEP_OUTPUT 1846 (insn_list 1842 (insn_list:REG_DEP_ANTI 1849 (nil)))))
>     (nil))
>
> eliminate_regs_in_insn() modification of the pseudo in the first
> expression does not affect the pseudo in the other expressions.

I must be dense or something.  But eliminate_regs_in_insn() is only for
converting eliminable hardregs into their target plus constant, like for
instance frame to stack pointer elimination.  The changing from pseudo
regs into their reg_renumber[] hardregs is done by alter_reg().  I.e. in
reload() after the
  for (i = LAST_VIRTUAL_REGISTER + 1...) alter_reg (i, -1);
loop, all pseudos which have a hardreg should be replaced by it (i.e. if
reg 929 got a hardreg it should be consistently replaced by
reg_renumber[929]).  I don't know how eliminate_regs_in_insn() should come
into play here (except maybe if r11 is somehow eliminable?).


Ciao,
Michael.

Re: state of 3.2.1-pre: how far from release?

[David Edelsohn]

>>>>> Michael Matz writes:

Michael> I must be dense or something.  But eliminate_regs_in_insn() is only for
Michael> converting eliminable hardregs into their target plus constant, like for
Michael> instance frame to stack pointer elimination.  The changing from pseudo
Michael> regs into their reg_renumber[] hardregs is done by alter_reg().
Michael> I don't know how eliminate_regs_in_insn() should come
Michael> into play here (except maybe if r11 is somehow eliminable?).

	reg 31 is the frame pointer, before elimination, so the first
force_reg moved a frame pointer offset address into a pseudo.
eliminate_regs_in_insn() first converts that instruction to:

(insn 1856 1853 16458 (parallel[ 
            (set (reg:SI 4 r4)
                (mem/s:SI (plus:SI (reg/f:SI 1 r1)
                        (const_int 520 [0x208])) [0 S4 A32]))
            (set (reg:SI 5 r5)
                (mem/s:SI (plus:SI (reg/f:SI 929)
                        (const_int 4 [0x4])) [0 S4 A32]))
            (set (reg:SI 6 r6)
                (mem/s:SI (plus:SI (reg/f:SI 929)
                        (const_int 8 [0x8])) [0 S4 A32]))
            (set (reg:SI 7 r7)
                (mem/s:SI (plus:SI (reg/f:SI 929)
                        (const_int 12 [0xc])) [0 S4 A32]))
        ] ) 322 {*rs6000.md:8764} (insn_list:REG_DEP_OUTPUT 1843 (insn_list:REG_DEP_OUTPUT 1846 (insn_list 1842 (insn_list:REG_DEP_ANTI 1849 (nil)))))
    (nil))

Note that only the pseudo in the first operand was modified.  Later

	(plus (reg 1) (const_int 520))

is reloaded into a reg because the instruction does not accept an
offsettable address.  Only the address in the first operand of the
parallel is substituted again.  I have been trying to show the first
instance where not all pseudos are replaced.

David

Re: state of 3.2.1-pre: how far from release?

[Michael Matz]

Hi,

On Wed, 6 Nov 2002, David Edelsohn wrote:

> 	reg 31 is the frame pointer, before elimination, so the first
> force_reg moved a frame pointer offset address into a pseudo.

Ok, nothing bad until now.

> eliminate_regs_in_insn() first converts that instruction to:

Does the insn before eliminate_regs_in_insn() only contains refs to (reg
929)?  And is reg_renumber[929] smaller zero, but reg_equiv_constant[929]
is non-zero (and points to the rtl (plus:SI (reg r1) (const_int 520))?  If
the answer to all these is yes, I think I know what happens, but not
exactly why.

The loop over all operands in eliminate_regs_in_insn() calls
eliminate_regs() for each operand (which is eliminable according to the
insn_data[] info), and places the results into substed_operand[].  If
eliminable_regs() is called on a pseudo reg (i.e. one which didn't get a
hard reg), but which has an equivalent constant (the (plus r31 520) in
this case, because of the REG_EQUIV note), then it returns that constant
(possibly recusively eliminating other regs).

Now the content of substed_operand[] is placed back into the places of the
old operands, but according to the recog_data information.  Because you
create that four-vector directly, and the matching insn pattern simply has
a (match_parallel) there, recog_data has no information about the other
operands, neither as normal operands, nor as (match_dup) like operands.
That's why the new rtl expression only is placed into the first operand
(for which there is a match_operand in the .md files).

As reload is working more or less only on operands as they are known by
recog(), I anyway wonder how such patterns are going to work, although I
might miss something.  Bug AFAIK reload never sees those additional
operands, and can't fix them up.  It's not reload itself which ICEs, but a
later pass, which really looks at the RTL, instead of just operands.

Just to confirm I would be interested which value recog_data.n_operands
and .n_dups have for this insn.

If it's the above reason, I'm not sure we want to fix this.  And if we do,
then probably not in reload, but in recog, in the sense that it fills
recog_data correctly with the additional operands.

I think the following way would work:

for a (match_parallel N PREDICATE [SUBPAT ...]) we additionally require
that not only the first corresponding elements of the parallel match the
SUBPAT, but _all_ of them (I'm looking at 3.2 docu, so that might be
outdated, or already required).  For match_operands in SUBPAT we would do
the obvious thing.  Given for instance this pattern:
(match_parallel 0 "bla"
 [(set (match_operand:SI 1 "blubb" "=r")
       (match_operand:SI 2 "blabla" "rm"))]

when matching this insn:
(parallel [
 (set (reg 1) (reg 2))
 (set (reg 3) (reg 4)) ] )

We would have operand 1 and 2 be (reg 1) resp. (reg 2), obviously, but
additionally we allocate more operands if needed (after setting all
explicitely mentioned match_operands), which would lead to operand 3 and 4
be (reg 3) and (reg 4).  I.e. for those things the operand number N in the
(match_operand) would be just a placeholder to be filled out by max
operand number till now plus N (or maybe also consecutively assigned,
but in relative order of the SUBPAT).

I'm not sure how difficult for recog that would be, and how many parts of
the compiler would be surprised to see different numbers of operands for
recog_data of the same insn code.  But if we would do that reload() would
magically start to work in this and similar situations.

That of course is nothing for 3.2, and probably not even 3.3.

> is reloaded into a reg because the instruction does not accept an
> offsettable address.  Only the address in the first operand of the
> parallel is substituted again.

Yep, the same reason as above.  reload() simply doesn't see those operands
at all.


Ciao,
Michael.

Re: state of 3.2.1-pre: how far from release?

[David Edelsohn]

>>>>> Michael Matz writes:

Michael> Does the insn before eliminate_regs_in_insn() only contains refs to (reg
Michael> 929)?  And is reg_renumber[929] smaller zero, but reg_equiv_constant[929]
Michael> is non-zero (and points to the rtl (plus:SI (reg r1) (const_int 520))?  If
Michael> the answer to all these is yes, I think I know what happens, but not
Michael> exactly why.

Yes,
	reg_renumber[929] = -1
and
	reg_equiv_constant[929] = (plus:SI (reg r31) (const_int 520))

Michael> The content of substed_operand[] is placed back into the places of the
Michael> old operands, but according to the recog_data information.  Because you
Michael> create that four-vector directly, and the matching insn pattern simply has
Michael> a (match_parallel) there, recog_data has no information about the other
Michael> operands, neither as normal operands, nor as (match_dup) like operands.
Michael> That's why the new rtl expression only is placed into the first operand
Michael> (for which there is a match_operand in the .md files).

	All instances of (reg/f:SI 929) are shared.  Why doesn't the
substition into the first operand affect all instances?  Or, another way
of stating it, why doesn't the substitution overwrite the RTL at the
pointer instead of changing the pointer of the first operand, at least for
match_parallel? 

David

Re: state of 3.2.1-pre: how far from release?

[Michael Matz]

Hi,

On Wed, 6 Nov 2002, David Edelsohn wrote:

> 	All instances of (reg/f:SI 929) are shared.

Doesn't matter in this case.

> Why doesn't the substition into the first operand affect all
> instances?

How could they?  the (reg 929) may be shared, but of course not the
 (plus:SI (reg 929) (const_int 4))  (for once, the constant is not the
same for them ;-) ) which are the operands here (not for recog though, but
they are supposed to be operands).  Ergo, if the first element of the
(plus) is changed to (plus (reg r1) (const 520)) (and the double plus
simplyfied), why should that change any of the other plus's?

There is some confusion here, because we right now call many things
substitution/elimination.  There is first the substitution of all pseudos
which got a hardreg with that hardreg (done in alter_reg(), and only here
matters the shared-ness of pseudo regs), which is simply done by
overwriting the REGNO integer in the (one an only) pseudo reg rtl.  But
this doesn't apply here anyway, reg 929 did not get a hardreg.

But we have other substitutions, and one of them is a part of
eliminate_regs(), in particular the substitution of pseudos which didn't
get hardregs with equivalent constants when they exist).  That hasn't much
to do with register elimination (like fp --> sp elim), but is done in the
same routine.  The way it is done is by simply replacing all operands
which are pseudos with their constants.  If some of the RTL isn't matched
by operands, we can't substitute it with the constant.

> Or, another way of stating it, why doesn't the substitution overwrite
> the RTL at the pointer instead of changing the pointer of the first
> operand, at least for match_parallel?

This can't be done.  We basically have this situation:
  op1 = (reg 929)
  op2 = (plus (reg 929) (const 4))
  op3 = (plus (reg 929) (const 8))

We also know, that (reg 929) == (plus (reg r11) 520).

Now how would you like to only change one of the three operands above, but
reflect that change automatically in the others too?  The only thing they
have in common are the (reg 929), ergo we would need to replace the
content of that rtx itself (like we overwrite the REGNO integer for the
pseudo-->hardreg conversion).  A REG rtx and a PLUS have both two
operands, so it _would_ possibly work, but then we have a shared (plus)
between the three operands, and can't change one without the others (for
any adjustments or whatever), so we would need to unshare them again,
which would again need accessing all three operands directly.  Besides
from the point that this is equivalent of directly replacing the first
element of the above (plus)'es (the reg) directly by (a copy of) the
constant.

No, that's never going to work, either the recog_data.operands[] array
(and the other things in recog_data) reflects _all_ real operands of an
instruction, or reload (and register elimination as we see here) needs to
be heavily changed (at least if this is supposed to work generally, maybe
there would be some hacks involving replace_rtx() to make this particular
case work).

As you said this works in HEAD, does anybody know if this issue really is
fixed there, or does it simply not happen there?


Ciao,
Michael.

Re: state of 3.2.1-pre: how far from release?

[David Edelsohn]

>>>>> Michael Matz writes:

Michael> This can't be done.  We basically have this situation:
Michael> op1 = (reg 929)
Michael> op2 = (plus (reg 929) (const 4))
Michael> op3 = (plus (reg 929) (const 8))

Michael> We also know, that (reg 929) == (plus (reg r11) 520).

	Your typos when copying the example are confusing the situation.

	(reg 929) = (plus (reg r31) (const_int 520))

r31 = frame pointer

However, the instruction does not allow an offsettable address as the
source, so the address sum eventually gets reloaded into a hard reg, e.g.,
r17.

	In other words, we eventually have

	(reg 929) = (reg r17)

replacing 929 with 17 can be performed by overwriting the reg.  The other
pluses are implicit in the instruction because the sequence of registers
is loaded from a sequence of memory addresses.

Michael> As you said this works in HEAD, does anybody know if this issue really is
Michael> fixed there, or does it simply not happen there?

	I do not see any changes in reload which would fix this.  HEAD
seems to have complete the substitution prior to elimination.  Is global
alloc just getting this right prior to reload?

David

Re: state of 3.2.1-pre: how far from release?

[Michael Matz]

Hi,

On Wed, 6 Nov 2002, David Edelsohn wrote:

> Michael> We also know, that (reg 929) == (plus (reg r11) 520).
>
> 	Your typos when copying the example are confusing the situation.

Bah.  Sorry ;)

> 	(reg 929) = (plus (reg r31) (const_int 520))
>
> r31 = frame pointer
>
> However, the instruction does not allow an offsettable address as the
> source, so the address sum eventually gets reloaded into a hard reg, e.g.,
> r17.

Yes, but that happens later.

> 	In other words, we eventually have
>
> 	(reg 929) = (reg r17)

No, we don't.  We have the first identity (p929 == r31+520) and then, when
reloading, for _one_ insn we have the identity
  r31+520 == r17.

From those two it does _not_ follow, that p929 is allocated to r17 as far
as reload is concerned.  It's of course true during that insn, but it
isn't equivalent to saying "reg_renumber[929] == 17".  One reason is, that
there is no p929 that reload sees in the insn, it just reloads the
expression r31+520 into some free hardreg.  The other is (even if reload
would see p929 somewhere) that _reloads_ are not done by directly changing
the pseudo reg rtx.  Doing that would be wrong, because a pseudo without
hardreg could be reloaded into hard X in one insn, and in hardreg Y in a
different insn and X is used in some other way in the latter insn.  But as
I said this anyway isn't important here, because the insn doesn't have
_any_ p929 anymore, as far as reload is concerned.

We basically have two different effects with the same reason here.  One is
that eliminate_regs_in_insn() does not change all occurences of p929 to
the constant expression.  That's not that bad, because it could be fixed
by reload later, if the same reason wouldn't also result in
the issue in reload, that it doesn't see those other operands.

> The other pluses are implicit in the instruction because the sequence

But there is noone knowing that.  The other operands don't exist at all,
therefore it's no wonder they end up being inconsistent with each other.

> of registers is loaded from a sequence of memory addresses.
>
> Michael> As you said this works in HEAD, does anybody know if this issue really is
> Michael> fixed there, or does it simply not happen there?
>
> 	I do not see any changes in reload which would fix this.  HEAD
> seems to have complete the substitution prior to elimination.

Hmm, before elimination, or is elimination doing the right thing?  (I.e.
changing all occurences of p929)

> Is global alloc just getting this right prior to reload?

If by coincidence that pseudo _does_ get a hardreg all would have worked
out (masking the error but still), so that would be possible.


Ciao,
Michael.

Re: state of 3.2.1-pre: how far from release?

[David Edelsohn]

>>>>> Michael Matz writes:

>> I do not see any changes in reload which would fix this.  HEAD
>> seems to have complete the substitution prior to elimination.

Michael> Hmm, before elimination, or is elimination doing the right thing?  (I.e.
Michael> changing all occurences of p929)

>> Is global alloc just getting this right prior to reload?

Michael> If by coincidence that pseudo _does_ get a hardreg all would have worked
Michael> out (masking the error but still), so that would be possible.

	Before elimination.  Compiling with HEAD, the testcase already has
hard regs assigned prior to the call to elimination.

David

Re: state of 3.2.1-pre: how far from release?

[Michael Matz]

Hi,

On Wed, 6 Nov 2002, David Edelsohn wrote:

> Michael> Hmm, before elimination, or is elimination doing the right thing?  (I.e.
> Michael> changing all occurences of p929)
>
> >> Is global alloc just getting this right prior to reload?
>
> Michael> If by coincidence that pseudo _does_ get a hardreg all would have worked
> Michael> out (masking the error but still), so that would be possible.
>
> 	Before elimination.  Compiling with HEAD, the testcase already has
> hard regs assigned prior to the call to elimination.

Then global-alloc allocated a hardreg, and that masks the error.  I bet if
you constrain it enough (by fixing some regs) you can make the same happen
again.


Ciao,
Michael.

Re: state of 3.2.1-pre: how far from release?

[Wolfgang Bangerth]

> We currently have 16 high-priority PRs against the 3.2 branch
> [...]
> There is no hope that the 3.2.1 release will be regression-free in the
> way that we are currently defining regressions: that *some* preceding

Volker Reichelt, I and several others have been sifting through a lot of
old reports recently, and are probably responsible for the many open "high
priority" C++ bugs. I think if we would go on doing so, we could go on
forever changing more reports into "high" mode than can be fixed. I think
hadn't we changed the priority of these reports, the question would not
even have arisen. So if you ask me, i.e. a simple user of gcc, then go
ahead and release the thing.

However, there is a thing that is really worrying me: there are presently
more than 1,800 non-closed reports, of which 570 are for C++ alone. I have
been looking at a lot of them, but the sheer number is creating problems:
I often think that this is a duplicate of something, but cannot seem to
find the other one, etc. That there are so many reports that nobody has
ever looked at is also the reason why we could turn up so many regressions
in so short a time.

If these numbers are allowed to grow, this will be getting out of hand. I 
think it would really be necessary for more people with "fixing capbility" 
to dig through the database and look what can be done. Just letting this 
grow and hoping that some volunteer eventually kills several dozens once 
in a while will not get us anywhere.

(By this I don't imply that gcc is in an overall bad shape. I think the
contrary is true. This is just a management problem, and it needs to be
addressed, because the database will become useless otherwise if there are
too many open reports. In this context, I can also only stress again that
it would be nice to have Bugzilla instead of GNATS, to make searching and
filtering simpler. Another feature that I am greatly missing is a button
to click, reading "I did confirm this report with recent CVS". This way I
could see whether someone looked at this recently or whether it might be
worthwhile re-checking some old report.)

Regards
  Wolfgang

-------------------------------------------------------------------------
Wolfgang Bangerth              email:           bangerth@ticam.utexas.edu
                               www: http://www.ticam.utexas.edu/~bangerth

Re: state of 3.2.1-pre: how far from release?

[Joe Buck]

> Volker Reichelt, I and several others have been sifting through a lot of
> old reports recently, and are probably responsible for the many open "high
> priority" C++ bugs. I think if we would go on doing so, we could go on
> forever changing more reports into "high" mode than can be fixed. I think
> hadn't we changed the priority of these reports, the question would not
> even have arisen. So if you ask me, i.e. a simple user of gcc, then go
> ahead and release the thing.

First, thanks for your work.

We have perhaps made a mistake, in the following sense: we are treating
all regressions equally.  By our present criteria, we can write a noise
generator, and if it crashes all gcc versions but one, we must call it
priority "high".

Since Mark asked to use priority "high" to indicate a regression, I won't
challenge him on that one, but perhaps we can use the severity field as
well.  At some point in the release process, we could consider only the
PRs with severity "critical", priority "high", and not marked as
"[mainline regression]" as blockers.

> However, there is a thing that is really worrying me: there are presently
> more than 1,800 non-closed reports, of which 570 are for C++ alone. I have
> been looking at a lot of them, but the sheer number is creating problems:
> I often think that this is a duplicate of something, but cannot seem to
> find the other one, etc.

We could certainly use more volunteers.

> If these numbers are allowed to grow, this will be getting out of hand.

The numbers can grow if releases are infrequent, because people will keep
re-reporting the same bugs.

> (By this I don't imply that gcc is in an overall bad shape. I think the
> contrary is true. This is just a management problem, and it needs to be
> addressed, because the database will become useless otherwise if there are
> too many open reports. In this context, I can also only stress again that
> it would be nice to have Bugzilla instead of GNATS, to make searching and
> filtering simpler. Another feature that I am greatly missing is a button
> to click, reading "I did confirm this report with recent CVS". This way I
> could see whether someone looked at this recently or whether it might be
> worthwhile re-checking some old report.)

Agreed about Bugzilla.  In the meantime, one possibility is to use the
modified-time in the advanced query form.  For PRs that haven't been
touched in a year, the volunteer can append a message to the audit trail:
either "confirmed present in x.y.z" or "no longer present".

Re: state of 3.2.1-pre: how far from release?

[Joseph S. Myers]

On Tue, 5 Nov 2002, Joe Buck wrote:

> Since Mark asked to use priority "high" to indicate a regression, I won't
> challenge him on that one, but perhaps we can use the severity field as
> well.  At some point in the release process, we could consider only the
> PRs with severity "critical", priority "high", and not marked as
> "[mainline regression]" as blockers.

Bugzilla will allow us to set milestones on bugs to distinguish
regressions which should be fixed in 3.2.1 / 3.2.2 / 3.3 / ... rather than
using a single priority "high" and out-of-band information to distinguish
which it means.

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

Re: state of 3.2.1-pre: how far from release?

[Paul Jarc]

Joe Buck <jbuck@synopsys.com> wrote:
> At some point in the release process, we could consider only the
> PRs with severity "critical", priority "high", and not marked as
> "[mainline regression]" as blockers.

My two cents: I'm not sure whether this is already covered by your
description, but I'd suggest that, for example, an ICE on incorrect
code should not be a blocker, even if it's a regression.  One failure
is as good as another for incorrect code, as far as I'm concerned.  At
the other end, wrong-code bugs are the scariest for me, since I build
my systems from source.  Wrong code can be go undetected and do much
damage over time.


paul

Re: state of 3.2.1-pre: how far from release?

[Joe Buck]

Paul Jarc writes:

> My two cents: I'm not sure whether this is already covered by your
> description, but I'd suggest that, for example, an ICE on incorrect
> code should not be a blocker, even if it's a regression.

Agreed.  The main reason that the rules are the way they are is that,
for point releases (e.g. 3.2.1) the purpose is only to fix regressions,
not to add new code to implement things that have never worked before.
So we can fix ICEs, for example.  It should not be a promise that all
regressions will be fixed, just that it is OK to fix them if we have a
fix.

Re: state of 3.2.1-pre: how far from release?

[Wolfgang Bangerth]

> We have perhaps made a mistake, in the following sense: we are treating
> all regressions equally.  By our present criteria, we can write a noise
> generator, and if it crashes all gcc versions but one, we must call it
> priority "high".

I think I came to this conclusion as well. However, I think the problem is 
that fixing these bugs came too late in the process. Many of these have 
been in the data base for a long time, and they need not be dug up only at 
the last minute.


> > However, there is a thing that is really worrying me: there are presently
> > more than 1,800 non-closed reports, of which 570 are for C++ alone. I have
> > been looking at a lot of them, but the sheer number is creating problems:
> > I often think that this is a duplicate of something, but cannot seem to
> > find the other one, etc.
> 
> We could certainly use more volunteers.

That's not the point. If I continue, I may have seen every C++ report in a 
month or so, but I will not recall the exact numbers and synopses of other 
bugs I have in mind for duplicates. Just throwing more people at that will 
not help, these bugs will have to be fixed to close them, also if we 
don't mark them as regressions.

On the other hand, there are so many that really need to be looked at 
by people with the right platform. I should start pinging more people if I 
cannot reproduce something on the platforms I have. And we should start 
writing the platform name into synopsis if this is not reproducable 
cross-platform.


> > If these numbers are allowed to grow, this will be getting out of hand.
> 
> The numbers can grow if releases are infrequent, because people will keep
> re-reporting the same bugs.

Sure, but the duplicates will not be closed and you end up with a database 
with a large amount of bugs. And the overall number of non-closed reports 
should not increase from release to release without bounds.


> Agreed about Bugzilla.  In the meantime, one possibility is to use the
> modified-time in the advanced query form.  For PRs that haven't been
> touched in a year, the volunteer can append a message to the audit trail:
> either "confirmed present in x.y.z" or "no longer present".

Is there a way to just append some text, or do I have to send mail for 
this. The latter is inconvenient with cut-and-pasting the synopsis, 
sometimes mail being spam-blocked, etc...

Regards
  Wolfgang

-------------------------------------------------------------------------
Wolfgang Bangerth              email:           bangerth@ticam.utexas.edu
                               www: http://www.ticam.utexas.edu/~bangerth

Re: state of 3.2.1-pre: how far from release?

[Ulrich Weigand]

David Edelsohn wrote:

> On the GCC 3.2 branch, reload_as_needed() is entered with the
>instruction still containing pseudos.  eliminate_regs_in_insn()
>substitutes a hard reg for the first use of the pseudo, but leaves the
>others untouched.  eliminate_regs_in_insn() recognizes the instruction and
>recog_data.n_operands is 3, so it only loops over three locations. 

We had the a similar problem on s390.  As I recall from the analysis
done at the time, this appeared to be a fundamental deficiency
in reload in that it simply would not perform replacements within
a match_parallel correctly.  I'm not sure how to fix this.

I've added a workaround to just disable the generation of any
load_multiple pattern that might need such replacements (i.e.
if an eliminable register occurs as part of the address).

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de