Re: egcs-19980803 (pre-1.1) solaris regressions

Re: egcs-19980803 (pre-1.1) solaris regressions

[Toshiyasu Morita]

> 
>    Date: Tue, 18 Aug 1998 10:42:04 -0600
>    From: Jeffrey A Law <law@hurl.cygnus.com>
> 
>    This is a bug in reorg.  I belive it is supposed to handle these
>    kinds of inconsistencies in the rtl and basic_block_* data
>    structures.
> 
> Actually I think it is a combination of bugs, 2 I can see at the
> moment:
> 
> 1) As you mention reorg is broken.  I walked through the example and
>    it does not find the basic blocks correctly and does not scan
>    the necessary instructions to find resource usages while scheduling
>    a delay instrution.  In this case, it does not set the ANNUL bit in
>    the instruction because of lost information.

I suppose it's a matter of perspective whether one regards flow as broken 
or reorg as broken, but from a practical standpoint of long-term code 
maintenance, it seems better to fix one compiler pass (flow) to provide 
accurate information instead of trying to kludge n other compiler passes 
to deal with inaccurate information.

I've been thinking about the NOTE_INSN_DELETED scheme, and it seems 
all compiler passes rely upon basic_block_head[n] to point to an actual 
insn and not a note, so a megapatch to modify all passes which are basic 
block information dependent would be required, which is probably bad.

It seems a scheme whereby new basic block information is introduced, but 
the current basic block information is retained would be better, since 
compiler passes could be converted one by one to utilize the new 
information without causing a flag day for everyone.

This could be done by introducing two new note types: NOTE_BASIC_BLOCK_BEGIN
and NOTE_BASIC_BLOCK_END, and providing corresponding arrays
new_basic_block_head[]/new_basic_block_end[], while retaining the current 
basic_block arrays. The compiler passes could be converted one by one 
over to the new basic block information, and when all passes are 
converted, the old basic block information could be removed.

Does this sound reasonable?

Toshi

Re: egcs-19980803 (pre-1.1) solaris regressions

[Jeffrey A Law]

  In message < 199808181650.JAA29001@dm.cobaltmicro.com >you write:
  > Actually I think it is a combination of bugs, 2 I can see at the
  > moment:
  > 
  > 1) As you mention reorg is broken.  I walked through the example and
  >    it does not find the basic blocks correctly and does not scan
  >    the necessary instructions to find resource usages while scheduling
  >    a delay instrution.  In this case, it does not set the ANNUL bit in
  >    the instruction because of lost information.
Yea.  This is nightmare code to deal with too.  The ability to clean
up this mess is one of the big wins if we recompute live/dead and
block start/end info after reload.

  > 2) Secondarily, I found another peculiar property of PIC during reload
  >    on Sparc.  When the reload insns are output, a reference to the
  >    PIC register (%l7) is created but I can't for the life of me figure
  >    out where the compiler accounts for this fact in the register
  >    tables in any way shape or form.  What gives?
Hmm.  Presumably the usage of the PIC register happens behind reload's
back.  ie, the pic reg is not a spill register and it's not already
mentioned elsewhere in the function before reload?

In that case I think it is the backend's responsibility to note that
the PIC register is in-use.

Note that having the PIC register come live behind reload's back can
lead to some problems.  We've run into this on the PA, m68k, x86 and ppc.

Hopefully your PIC register is fixed and not a pseudo?  And hopefully,
you always allocate a stack slot for it when -fpic/-fPIC?



targ

Re: egcs-19980803 (pre-1.1) solaris regressions

[David S. Miller]

   Date: Tue, 18 Aug 1998 11:03:38 -0600
   From: Jeffrey A Law <law@hurl.cygnus.com>

   Yea.  This is nightmare code to deal with too.  The ability to clean
   up this mess is one of the big wins if we recompute live/dead and
   block start/end info after reload.

Indeed, and I still want to implement pre-delay slots for sake of
Sparc FPU branches as well...

   Hmm.  Presumably the usage of the PIC register happens behind reload's
   back.  ie, the pic reg is not a spill register and it's not already
   mentioned elsewhere in the function before reload?

It gets used behind reloads back, and it is acquired for register
allocation as well.  It is fine to use it normally before register
allocation, but if reload_in_progress we should note what we are
doing.

   In that case I think it is the backend's responsibility to note that
   the PIC register is in-use.

So I should update basic_block_live_at_start, or other things as well?

   Note that having the PIC register come live behind reload's back can
   lead to some problems.  We've run into this on the PA, m68k, x86 and ppc.

Do other ports handle this by updating the alloc/reload tables?  I
should go check what other people do wrt. this.  I don't want to mark
it as not allocatable at all.  (a quick check shows that ix86 marks it
as fixed, aieee thats nuts!)

   Hopefully your PIC register is fixed and not a pseudo?  And hopefully,
   you always allocate a stack slot for it when -fpic/-fPIC?

It is fixed (but rth and I want to change this so functions which use
PIC can still be made leaf functions).  I need to go over how we do
all of this on Sparc, LEGITIMATE_PIC_OPERAND_P seems suspicious at the
moment and actually could be the true root of the problem I am seeing
here.

   targ

An emacs abbrev-mode fart? :-)

Later,
David S. Miller
davem@dm.cobaltmicro.com

Re: egcs-19980803 (pre-1.1) solaris regressions

[Jeffrey A Law]

  In message < 199808181749.KAA02644@netcom8.netcom.com >you write:
  > I suppose it's a matter of perspective whether one regards flow as broken 
  > or reorg as broken, but from a practical standpoint of long-term code 
  > maintenance, it seems better to fix one compiler pass (flow) to provide 
  > accurate information instead of trying to kludge n other compiler passes 
  > to deal with inaccurate information.
Well, until very recently we didn't even have the option of trying to
fix reorg.

I'll go on record now that I would prefer to see the basic block data
structures and such accurate after reload, even if that means we have
to run a mini dataflow analysis or flesh out more of the live range
splitting support code.  I think that's cleaner than a mega patch and
magic NOTE_INSN_DELETED notes.

Note that this will not be for egcs-1.1; making this stuff accurate
should happen on the mainline tree.


jeff

Re: egcs-19980803 (pre-1.1) solaris regressions

[David S. Miller]

   Date: Tue, 18 Aug 1998 19:07:29 -0600
   From: Jeffrey A Law <law@cygnus.com>

   I'll go on record now that I would prefer to see the basic block
   data structures and such accurate after reload, even if that means
   we have to run a mini dataflow analysis or flesh out more of the
   live range splitting support code.  I think that's cleaner than a
   mega patch and magic NOTE_INSN_DELETED notes.

It can be fixed in a more localized and less intrusive way, by
catching these cases in emit-rtl.c and it can be done at low cost as
well.  I did it in my local sources while investigating this bug.

Later,
David S. Miller
davem@dm.cobaltmicro.com

Re: egcs-19980803 (pre-1.1) solaris regressions

[Jeffrey A Law]

  In message < 199808191539.IAA05396@dm.cobaltmicro.com >you write:
  >    Date: Tue, 18 Aug 1998 19:07:29 -0600
  >    From: Jeffrey A Law <law@cygnus.com>
  > 
  >    I'll go on record now that I would prefer to see the basic block
  >    data structures and such accurate after reload, even if that means
  >    we have to run a mini dataflow analysis or flesh out more of the
  >    live range splitting support code.  I think that's cleaner than a
  >    mega patch and magic NOTE_INSN_DELETED notes.
  > 
  > It can be fixed in a more localized and less intrusive way, by
  > catching these cases in emit-rtl.c and it can be done at low cost as
  > well.  I did it in my local sources while investigating this bug.
Seems like a reasonable approach too.  Certainly worth looking at.

jeff

Re: egcs-19980803 (pre-1.1) solaris regressions

[Jeffrey A Law]

  In message < 199808181205.FAA23454@dm.cobaltmicro.com >you write:
  >    Date: Fri, 7 Aug 1998 16:28:45 -0400 (EDT)
  >    From: "Kaveh R. Ghazi" <ghazi@caip.rutgers.edu>
  > 
  > Hi Kaveh,
  > 
  > 	   The following errors (except for 930628-1) are regressions
  >    from prior releases/snapshots, but they don't appear in "make check"
  >    because the testsuite doesn't run with -fpic/-fPIC.
  > 
  >    ERROR: 930628-1.c: compiled program got signal 11: -O -fno-omit-frame-po
  > inter -fpic
  >    ERROR: 930628-1.c: compiled program got signal 11: -O -fomit-frame-point
  > er -fpic
  > 
  > Interesting, as I have begun to look into these tonight.  The cause of
  > all these errors is the same thing, and it is related to another
  > recent thread on here from which I will quote the following:
  > 
[ ... ]
  > No Jeff, that is incorrect, it is a _serious_ problem.  Something bad
  > does in fact happen if the basic blocks get spilled over like this.
  > 
  > For one, and this is the failure mode we are seeing on Sparc with
  > Kaveh's PIC test case failures, reorg can't figure out the correct
  > basic block to re-record liveness information when filling delay
  > slots, and as a result the lifetime info becomes corrupt and thus will
  > cause reorg to clobber registers illegally.  For example, in one of
  > the Sparc PIC testcases mentioned above reorg changes:
This is a bug in reorg.  I belive it is supposed to handle these kinds
of inconsistencies in the rtl and basic_block_* data structures.

jeff

Re: egcs-19980803 (pre-1.1) solaris regressions

[David S. Miller]

   Date: Fri, 7 Aug 1998 16:28:45 -0400 (EDT)
   From: "Kaveh R. Ghazi" <ghazi@caip.rutgers.edu>

Hi Kaveh,

	   The following errors (except for 930628-1) are regressions
   from prior releases/snapshots, but they don't appear in "make check"
   because the testsuite doesn't run with -fpic/-fPIC.

   ERROR: 930628-1.c: compiled program got signal 11: -O -fno-omit-frame-pointer -fpic
   ERROR: 930628-1.c: compiled program got signal 11: -O -fomit-frame-pointer -fpic

Interesting, as I have begun to look into these tonight.  The cause of
all these errors is the same thing, and it is related to another
recent thread on here from which I will quote the following:

>   In message < 199808151124.EAA24081@netcom8.netcom.com >you write:
>   > I'm seeing some strange problems with reload and I think it may have 
>   > something to do with basic_block_head[] and basic_block_tail[]...
>   > 
>   > Specifically, the current implementation of basic_block_head[] and 
>   > basic_block_tail[] seem to point at actual insns. This seems bad
>   > because reload calls emit_insn_before() and emit_insn_after(), and these 
>   > functions seem unaware of the existence of the basic_block_head/tail arrays.
>   > 
>   > Consider: if reload generates a spill for the first insn of a 
>   > basic block, it seems to insert the insns into the basic block outside 
>   > the one intended, or even outside the outermost basic block, like this:
> It's a problem, but not a serious one.  Nothing bad should happen as
> far as I know.

(The bulk of the quoted text is from Toshiyasu Marita, the final
comment is from Jeff Law).

No Jeff, that is incorrect, it is a _serious_ problem.  Something bad
does in fact happen if the basic blocks get spilled over like this.

For one, and this is the failure mode we are seeing on Sparc with
Kaveh's PIC test case failures, reorg can't figure out the correct
basic block to re-record liveness information when filling delay
slots, and as a result the lifetime info becomes corrupt and thus will
cause reorg to clobber registers illegally.  For example, in one of
the Sparc PIC testcases mentioned above reorg changes:

	bne .LL36
	 nop

	ld [%l7+.LLC0],%o2
	ld [%o2],%f2
	st %f2,[%i5+%i3]
	b .LL18
	 nop
.LL36:
	mov 0,%l7
	sll %l0,4,%o0

into:

	bne .LL36
	mov 0,%l7		<=== This is illegal

	ld [%l7+.LLC0],%o2	<=== whoops, we crash here now
	ld [%o2],%f2
	b .LL18
	st %f2,[%i5+%i3]
.LL25:
	mov 0,%l7
.LL36:
	sll %l0,4,%o0

And it is because reload emits:

;; End of basic block 6

(note 340 51 349 "" NOTE_INSN_DELETED)

;; Insn is not within a basic block
(insn 349 340 350 (set (reg:SI 10 %o2)
        (mem/u:SI (plus:SI (reg:SI 23 %l7)
                (symbol_ref/u:SI ("*.LLC0"))))) 113 {*movsi_insn} (nil)
    (expr_list:REG_EQUAL (symbol_ref/u:SI ("*.LLC0"))
        (nil)))

(note 350 349 353 "" NOTE_INSN_DELETED)

;; Insn is not within a basic block
(insn 353 350 75 (set (reg:SF 34 %f2)
        (mem:SF (reg:SI 10 %o2))) 124 {*movsf_insn} (nil)
    (nil))

;; Start of basic block 7, registers live: 14 [%sp] 30 [%fp] 31 [%i7] 105 106 107 108 114 117 120 121 122 125 126 132
(insn:HI 75 353 76 (set (mem/s:SF (plus:SI (reg:SI 29 %i5)
                (reg:SI 27 %i3)))
        (reg:SF 34 %f2)) 124 {*movsf_insn} (nil)
    (nil))

(jump_insn 76 75 77 (set (pc)
        (label_ref 185)) 360 {jump} (nil)
    (nil))

The insn 75 needed a reload to form the address.  Reload emitted the
reload insns before it into spots before bblock 7 begins (as Toshi has
explained it will do).  Later reorg says "aha, %l7 is not live, the
clr of it in the target of this branch, so I can put the clear of it
in the delay slot."

Voila :-(  I am really surprised we've never been bitten by this
before.

Later,
David S. Miller
davem@dm.cobaltmicro.com

egcs-19980803 (pre-1.1) solaris regressions

[Kaveh R. Ghazi]

Snapshot:		egcs-19980803 (1.1 prerelease snapshot)
Platform:		sparc-sun-solaris2.5
Stage1 compiler:	cc
Assembler:		native
Linker:			native
BOOT_CFLAGS:		-g -O3 -funroll-all-loops


	The following errors (except for 930628-1) are regressions
from prior releases/snapshots, but they don't appear in "make check"
because the testsuite doesn't run with -fpic/-fPIC.

	I got these with my modified c-torture (upon which I run
-fpic/-fPIC.)  And the files from my c-torture/execute directory are
the same as in the egcs/gcc/testsuite dir.  So you can just use those
files with the right flags to reproduce this problem.

		--Kaveh

ERROR: 960215-1.c: compiled program got signal 11: -O2 -fno-omit-frame-pointer -fpic
ERROR: 960215-1.c: compiled program got signal 11: -O2 -fomit-frame-pointer -fpic
ERROR: 960215-1.c: compiled program got signal 11: -O2 -funroll-all-loops -fpic
ERROR: 960215-1.c: compiled program got signal 11: -O3 -fno-omit-frame-pointer -fpic
ERROR: 960215-1.c: compiled program got signal 11: -O3 -fomit-frame-pointer -fpic
ERROR: 960215-1.c: compiled program got signal 11: -O3 -funroll-all-loops -fpic
ERROR: 930628-1.c: compiled program got signal 11: -O -fno-omit-frame-pointer -fpic
ERROR: 930628-1.c: compiled program got signal 11: -O -fomit-frame-pointer -fpic

ERROR: rbug.c: compiled program got signal 6: -O3 -fno-omit-frame-pointer -fpic
ERROR: rbug.c: compiled program got signal 6: -O3 -fomit-frame-pointer -fpic
ERROR: rbug.c: compiled program got signal 6: -O3 -funroll-all-loops -fpic




ERROR: 960215-1.c: compiled program got signal 11: -O2 -fno-omit-frame-pointer -fPIC
ERROR: 960215-1.c: compiled program got signal 11: -O2 -fomit-frame-pointer -fPIC
ERROR: 960215-1.c: compiled program got signal 11: -O2 -funroll-all-loops -fPIC
ERROR: 960215-1.c: compiled program got signal 11: -O3 -fno-omit-frame-pointer -fPIC
ERROR: 960215-1.c: compiled program got signal 11: -O3 -fomit-frame-pointer -fPIC
ERROR: 960215-1.c: compiled program got signal 11: -O3 -funroll-all-loops -fPIC
ERROR: 930628-1.c: compiled program got signal 11: -O -fno-omit-frame-pointer -fPIC
ERROR: 930628-1.c: compiled program got signal 11: -O -fomit-frame-pointer -fPIC

ERROR: rbug.c: compiled program got signal 6: -O3 -fno-omit-frame-pointer -fPIC
ERROR: rbug.c: compiled program got signal 6: -O3 -fomit-frame-pointer -fPIC
ERROR: rbug.c: compiled program got signal 6: -O3 -funroll-all-loops -fPIC
--
Kaveh R. Ghazi			Project Manager / Custom Development
ghazi@caip.rutgers.edu		Icon CMT Corp.