Bug 11052 - [3.3 regression] [arm] noce_process_if_block() can loose REG_INC notes
Summary: [3.3 regression] [arm] noce_process_if_block() can loose REG_INC notes
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 3.3
: P1 critical
Target Milestone: 3.3.1
Assignee: Not yet assigned to anyone
URL:
Keywords: ice-on-valid-code
: 10287 (view as bug list)
Depends on:
Blocks:
 
Reported: 2003-06-01 11:16 UTC by Debian GCC Maintainers
Modified: 2003-06-13 10:56 UTC (History)
3 users (show)

See Also:
Host: arm-linux
Target: arm-linux
Build: arm-linux
Known to work:
Known to fail:
Last reconfirmed: 2003-06-02 10:19:14


Attachments
preprocessed source (40.64 KB, text/plain)
2003-06-01 11:17 UTC, Debian GCC Maintainers
Details
reduced testcase.i (523 bytes, text/plain)
2003-06-02 10:18 UTC, Dara Hazeghi
Details
noce_sideeffect.patch (558 bytes, text/plain)
2003-06-06 17:23 UTC, Richard Earnshaw
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Debian GCC Maintainers 2003-06-01 11:16:29 UTC
This is a regression from 2.95 and 3.2; works with HEAD 20030531.  Compiling
with -O1 makes the ICE go away.

gcc -c -fsigned-char    -I../.. -I../../exports/include   -Dlinux -D__arm__
-D__arm32__ -U__arm -Uarm -D_POSIX_C_SOURCE=199309L -D_POSIX_SOURCE
-D_XOPEN_SOURCE -D_BSD_SOURCE -D_SVID_SOURCE  -D_GNU_SOURCE   -DFUNCPROTO=15
-DNARROWPROTO -DXTHREADS  -D_REENTRANT -DXUSE_MTSAFE_API   
-DMALLOC_0_RETURNS_NULL  -DHAS_SNPRINTF -DLIBX11  -g -O2 -g  Xrm.c -o
unshared/Xrm.o 
Xrm.c: In function `GetDatabase': 
Xrm.c:1522: internal compiler error: Segmentation fault 
Please submit a full bug report, 
with preprocessed source if appropriate. 
See <URL:http://gcc.gnu.org/bugs.html> for instructions. 
make[5]: *** [Xrm.o] Error 1
Comment 1 Debian GCC Maintainers 2003-06-01 11:17:45 UTC
Created attachment 4122 [details]
preprocessed source
Comment 2 Dara Hazeghi 2003-06-02 10:18:12 UTC
Created attachment 4135 [details]
reduced testcase.i
Comment 3 Dara Hazeghi 2003-06-02 10:19:12 UTC
Confirmed with gcc 3.3 branch (20030531). Note that this problem does not occur on arm-elf... I 
have somewhat reduced the testcase, and included it. I've verified that the backtrace of the crash 
of the new testcase is the same to that of the old one (well, almost completely the same). Looks 
like move2add_note_store is getting passed NULL data from note_stores, and that in turn is 
getting NULL data from reload_cse_move2add. Hope this information is helpful to someone :-)

Dara

Backtrace:
0x00237b9c in move2add_note_store (dst=0xe7b0c0, set=0xe79654, data=0x0) at 
reload1.c:9293
9293            reg_set_luid[REGNO (XEXP (dst, 0))] = 0;
(gdb) bt
#0  0x00237b9c in move2add_note_store (dst=0xe7b0c0, set=0xe79654, data=0x0) at 
reload1.c:9293
#1  0x001c6afc in note_stores (x=0xe79654, fun=0x237a64 <move2add_note_store>, data=0x0) 
at rtlanal.c:1637
#2  0x002379b0 in reload_cse_move2add (first=0xe2b8f0) at reload1.c:9249
#3  0x00233c28 in reload_cse_regs (first=0xe2b8f0) at reload1.c:8154
#4  0x000b0ecc in rest_of_compilation (decl=0xe62620) at toplev.c:3335
#5  0x00020670 in c_expand_body (fndecl=0xe62620, nested_p=0, can_defer_p=1) at c-
decl.c:6567
#6  0x00020250 in finish_function (nested=0, can_defer_p=1) at c-decl.c:6407
#7  0x00003bf4 in yyparse () at c-parse.y:403
Comment 4 Richard Earnshaw 2003-06-02 17:09:55 UTC
Subject: Re:  [3.3 regression] [arm] ICE (segfault) 
 compiling xfree86

Bizzare.  Your analysis does not match mine at all (though I am getting a 
segfault).

Firstly, I get this on arm-elf as well as on arm-linux (but not if I have 
-mcpu=strongarm on the options list).

What I am seeing happening is that we have the construct

    int quarks[100 + 1];
    register int num_quarks;

        num_quarks = 0;

        for(;;) {
                        quarks[num_quarks++] = something;
	}

And this is being transformed into (roughly):


    int quarks[100 + 1];
    int *q_ptr = quarks;

    for (;;) {
	*q_ptr++ = something;
    }

Or, in rtl:

(insn 193 23 30 0 (nil) (set (reg/v/f:SI 39)
        (reg/f:SI 73)) 175 {*arm_movsi_insn} (insn_list 23 (nil))
    (expr_list:REG_EQUAL (plus:SI (reg/f:SI 25 sfp)
            (const_int -9000 [0xffffdcd8]))
        (nil)))

...

(insn 94 162 95 6 0x4001d7bc (set (mem:SI (pre_inc:SI (reg/v/f:SI 39)) [8 
S4 A32
])
        (reg:SI 54)) 175 {*arm_movsi_insn} (nil)
    (expr_list:REG_INC (reg/v/f:SI 39)
        (expr_list:REG_DEAD (reg:SI 54)
            (nil))))

Now during local register allocation that REG_EQUAL note on insn 193 is 
being transformed into a REQ_EQUIV (which implies that 39 never changes).  
So when reload fails to allocate a register to reg 39 it simply substitues 
in the equiv expression.  That leaves us with a post_inc of something that 
isn't a register and has no stack equivalent.

(insn:HI 211 210 103 5 0x401d52ec (set (mem:SI (pre_inc:SI (plus:SI 
(reg/f:SI 11 fp)
                    (const_int -9040 [0xffffdcb0]))) [8 S4 A32])
        (reg:SI 3 r3 [93])) 124 {*arm_movsi_insn} (insn_list 210 (nil))
    (expr_list:REG_DEAD (reg:SI 3 r3 [93])
        (expr_list:REG_INC (plus:SI (reg/f:SI 11 fp)
                (const_int -9040 [0xffffdcb0]))
            (nil))))

We then get confused in a later pass when we try to kill an auto-inc value 
because an auto-inc should never apply to anything other than a register.

I need to look into this further, but it appears that REG_N_SETS hasn't 
taken into account the auto_inc operation ;-(

R.

Comment 5 Dara Hazeghi 2003-06-02 17:34:15 UTC
Well, you're the ARM maintainer, so I suspect your analysis is much closer... Also could be because 
I'm using cross compilers (sorry, very important details)...
Comment 6 Richard Earnshaw 2003-06-03 12:25:14 UTC
Subject: Re:  [3.3 regression] [arm] ICE (segfault) 
 compiling xfree86

> Well, you're the ARM maintainer, so I suspect your analysis is much closer... Also could be because 
> I'm using cross compilers (sorry, very important details)...

I'm not saying you are wrong, just that I'm not seeing what you are 
seeing.  Which concerns me somewhat.   And I was using a cross-compiler 
too...

Is there any chance you could have another look at the failure you are 
seeing and tell me what the "insn" is in frame 2

#2  0x002379b0 in reload_cse_move2add (first=0xe2b8f0) at reload1.c:9249

reload.c:9249:  note_stores (PATTERN (insn), move2add_note_store, NULL);

R.

Comment 7 Richard Earnshaw 2003-06-03 15:13:34 UTC
Subject: Re:  [3.3 regression] [arm] ICE (segfault) 
 compiling xfree86


rearnsha@arm.com said:
> I need to look into this further, but it appears that REG_N_SETS
> hasn't  taken into account the auto_inc operation ;-( 

The key to this is that if-then-else conversion has dropped a REG_INC 
note.  Prior to conversion we had the following code paths

			+-----------+
			| cmp x, y  |
			+-----------+
			/           \
		+-----------+     +-----------+
		| t1 = 1    |     | t2 = 0    |
		| *++p = t1 |     | *++p = t2 |
		+-----------+     +-----------+
			\           /
			+-----------+
			|    ...    |
			+-----------+


If-then-else conversion is converting this to

		cmp x, y
		t3 = cond ? 1 : 0
		*++p = t3

In doing so it generates a new insn for the assignment, but fails to copy 
the REG_INC note.  Thus when mark_set_regs is called to update the reg 
life info it fails to note that p is updated.  This then causes p to be 
considered a constant and as a final consequence for p to be entirely 
eliminated (by reload) into a frame address.

This all starts to go wrong in noce_process_if_block.  It seems to me that 
there are three possibilities.

1) Don't allow this transformation if the insns have REG_INC notes (this 
is overkill, but safe).
2) Copy any REG_INC notes from *one* of the source insns.
3) Something else I've not thought of... :-)

Richard, any thoughts?

R.

Comment 8 Richard Henderson 2003-06-03 16:12:23 UTC
Subject: Re:  [3.3 regression] [arm] ICE (segfault) compiling xfree86

On Tue, Jun 03, 2003 at 04:13:09PM +0100, Richard Earnshaw wrote:
> 1) Don't allow this transformation if the insns have REG_INC notes (this 
> is overkill, but safe).

This I'd recommend for 3.3, if it's affected.

> 2) Copy any REG_INC notes from *one* of the source insns.

This shouldn't be hard, I wouldn't expect.

> 3) Something else I've not thought of... :-)

Build a new REG_INC note after scanning for autoinc codes?


r~
Comment 9 Richard Earnshaw 2003-06-03 16:52:25 UTC
Subject: Re:  [3.3 regression] [arm] ICE (segfault) 
 compiling xfree86

> On Tue, Jun 03, 2003 at 04:13:09PM +0100, Richard Earnshaw wrote:
> > 1) Don't allow this transformation if the insns have REG_INC notes (this 
> > is overkill, but safe).
> 
> This I'd recommend for 3.3, if it's affected.
> 
> > 2) Copy any REG_INC notes from *one* of the source insns.
> 
> This shouldn't be hard, I wouldn't expect.
> 
> > 3) Something else I've not thought of... :-)
> 
> Build a new REG_INC note after scanning for autoinc codes?
> 

Is it guaranteed that noce_emit_move_insn will emit exactly one insn?

R.

Comment 10 Dara Hazeghi 2003-06-03 17:52:53 UTC
Richard,

you're right, I do get the failure on arm-elf now. I'm not sure why I didn't before. If you still need 
the info in frame 2, I can dig it up (with some minimal instructions, I'm quite new to gdb)...

Dara
Comment 11 Richard Henderson 2003-06-03 22:24:33 UTC
Subject: Re:  [3.3 regression] [arm] ICE (segfault) compiling xfree86

On Tue, Jun 03, 2003 at 05:52:16PM +0100, Richard Earnshaw wrote:
> Is it guaranteed that noce_emit_move_insn will emit exactly one insn?

No.


r~
Comment 12 Richard Earnshaw 2003-06-06 17:23:35 UTC
Subject: Re:  [3.3 regression] [arm] ICE (segfault) 
 compiling xfree86

> On Tue, Jun 03, 2003 at 05:52:16PM +0100, Richard Earnshaw wrote:
> > Is it guaranteed that noce_emit_move_insn will emit exactly one insn?
> 
> No.
> 
> 

In that case, and given that on further review of the source it becomes 
apparent that two of the three variants handled by noce_process_if_block 
lead to invalid code if X has side effects, I think it's easier to just 
disable this optimization for that case.

So fixed with:


2003-06-06  Richard Earnshaw  <rearnsha@arm.com>

	PR target/11052
	* ifcvt.c (noce_process_if_block): Fail if the destination has
	side-effects.

2003-06-06  Richard Earnshaw  <rearnsha@arm.com>

	gcc.c-torture/execute/20030606-1.c: New.


Comment 13 Richard Earnshaw 2003-06-06 17:23:36 UTC
Created attachment 4183 [details]
noce_sideeffect.patch
Comment 14 Richard Earnshaw 2003-06-06 17:45:41 UTC
Fixed.
Comment 15 Richard Earnshaw 2003-06-13 10:56:51 UTC
*** Bug 10287 has been marked as a duplicate of this bug. ***
Comment 16 Daniel Jacobowitz 2003-06-17 15:58:52 UTC
Subject: Re:  [3.3 regression] [arm] noce_process_if_block() can loose REG_INC notes

Yes, it looks like your patch fixed the test case.  Thanks!

Before I write it off as closed, do you have any comments on this bit:

  > It was then broken again in HEAD by:
  > Wed Jan  8 12:10:57 CET 2003  Jan Hubicka  <jh@suse.cz>
  > 
  >        * i386.md (adddi3_carry_rex64, subdi3_carry_rex64): Name pattern.
  >        (addhi3_carry, addqi3_carry, subhi3_carry, subqi3_carry): New patterns.
  >        (add??cc): New expanders.
  >        * i386.c (expand_int_addcc): New function.
  >        * i386-protos.h (expand_int_addcc): Declare.
  > 
  >        * alias.c (memory_modified_1): New static function.
  >        (memory_modified): New static varaible.
  >        (memory_modified_in_insn_p): New global function.
  >        * rtl.h (memory_modified_in_insn_p): Declare.
  >        * rtlanal.c (modified_between_p, modified_in_p): Be smart about memory
  >        references.
  > 
  >        * expr.h (emit_conditional_add): Declare.
  
  I'm not sure, but I think that modified_between_p and modified_in_p are
  going to have to have POST_INC (POST_DEC, PRE_INC, PRE_DEC) cases in
  them for the above patch from Jan to be safe.

I'd hate for it to come back and bite me later.