Bug 17019 - THUMB: bad switch statement in md code for addsi3_cbranch_scratch
THUMB: bad switch statement in md code for addsi3_cbranch_scratch
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: target
3.4.1
: P2 normal
: 3.4.2
Assigned To: Richard Earnshaw
: wrong-code
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-08-13 16:42 UTC by Dan Bornstein
Modified: 2004-08-17 11:33 UTC (History)
1 user (show)

See Also:
Host:
Target: arm-unknown-elf
Build:
Known to work:
Known to fail: 3.4.0
Last reconfirmed: 2004-08-16 14:53:29


Attachments
proposed patch (373 bytes, patch)
2004-08-13 21:36 UTC, Dan Bornstein
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Bornstein 2004-08-13 16:42:50 UTC
Compiling the following with -O1 for thumb results in bogus code being generated:

    void badness(int a)
    {
        void zorch(int b);
        int b;

        for (b = -1; b < a + 1; b++) {
            zorch(b);
        }
    }

In particular, it looks like the loop prologue code is screwed up and will cause the loop to always be 
skipped.

Here's how I configured and built the compiler:

$ tar -xjvf gcc-core-3.4.0.tar.bz2
$ cd gcc-3.4.0
$ ./configure --prefix=/usr/local/armdev --target=arm-elf --with-newlib --enable-languages=c

The following lines were uncommented in gcc/config/arm/t-arm-elf:

MULTILIB_OPTIONS    += mno-thumb-interwork/mthumb-interwork
MULTILIB_DIRNAMES   += normal interwork
MULTILIB_EXCEPTIONS += *mapcs-26/*mthumb-interwork*

$ make
$ sudo make install

In case it matters, I'm currently using binutils-2.15.

I will try this with 3.4.1 shortly.
Comment 1 Dan Bornstein 2004-08-13 16:45:23 UTC
Sorry, forgot to put in the explicit compile line. Here it is:

arm-elf-gcc -save-temps -mthumb -O1 -c blort.c
Comment 2 Dan Bornstein 2004-08-13 17:18:09 UTC
The bug still seems to happen with 3.4.1. For the record, here's the assemby code that I think is in 
error:

badness:
        push    {r4, r5, lr}
        mov     r4, #1
        neg     r4, r4
        // probably missing a test here
        bmi     .L7

That is, the code correctly initializes r4 to -1, but then always branches around the loop because the 
branch condition is always true.
Comment 3 Dan Bornstein 2004-08-13 17:54:46 UTC
I just looked at the intermediate RTL, and, near as I can tell, as of pass 35.mach, the code is ok. Here's 
the loop skip test:

(jump_insn 44 74 72 (parallel [
            (set (pc)
                (if_then_else (lt (plus:SI (reg/v:SI 0 r0 [orig:68 a ] [68])
                            (const_int 1 [0x1]))
                        (const_int 0 [0x0]))
                    (label_ref 45)
                    (pc)))
            (clobber (reg:SI 3 r3))
        ]) 176 {*addsi3_cbranch_scratch} (insn_list 3 (nil))
    (expr_list:REG_UNUSED (reg:SI 3 r3)
        (expr_list:REG_BR_PROB (const_int 3600 [0xe10])
            (nil))))

I read that as skipping the loop if (a + 1) < 0, which I believe is correct.
Comment 4 Dan Bornstein 2004-08-13 20:07:24 UTC
If I compile with -dP, I find that the bmi instruction gets annotated like this:

@(jump_insn 44 74 72 (parallel [
@            (set (pc)
@                (if_then_else (lt (plus:SI (reg/v:SI 0 r0 [orig:68 a ] [68])
@                            (const_int 1 [0x1]))
@                        (const_int 0 [0x0]))
@                    (label_ref 45)
@                    (pc)))
@            (clobber (reg:SI 3 r3))
@        ]) 176 {*addsi3_cbranch_scratch} (insn_list 3 (nil))
@    (expr_list:REG_UNUSED (reg:SI 3 r3)
@        (expr_list:REG_BR_PROB (const_int 3600 [0xe10])
@            (nil))))
@ 0x0004
        bmi     .L7     @ 44    *addsi3_cbranch_scratch/3       [length = 4]

This seems to correspond to arm.md around line 6211.

Assuming that that is indeed the right pattern to match, then it looks like either there's a missing case 
in the switch statement or the which_alternative variable got set incorrectly.
Comment 5 Dan Bornstein 2004-08-13 20:11:23 UTC
I wrote:
>Assuming that that is indeed the right pattern to match, then it looks like either there's a missing
>case in the switch statement or the which_alternative variable got set incorrectly.

I just checked, and it looks like which_alternative is 2 in this case, which is not one of the cases of the 
switch.
Comment 6 Dan Bornstein 2004-08-13 21:23:46 UTC
After giving myself a crash course on gcc md file syntax, I was convinced that the switch statement 
cases are actually supposed to be 0-3 as opposed to skipping 2. I made the change and it looks like the 
problem went away, at least from the test case I submitted with this bug.
Comment 7 Dan Bornstein 2004-08-13 21:36:03 UTC
Created attachment 6922 [details]
proposed patch
Comment 8 Richard Earnshaw 2004-08-16 14:53:28 UTC
I agree with your analysis.

I'm just running a test to see if this patch fixes another problem I was in the
early stages of trying to track down.

Thanks for the detailed bug report.
Comment 9 CVS Commits 2004-08-17 10:01:56 UTC
Subject: Bug 17019

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	rearnsha@gcc.gnu.org	2004-08-17 10:01:50

Modified files:
	gcc            : ChangeLog 
	gcc/config/arm : arm.md 

Log message:
	From Daniel Bornstein  <danfuzz@milk.com>
	PR target/17019
	* arm.md (addsi3_cbranch_scratch): Correct case labels.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.4932&r2=2.4933
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/arm/arm.md.diff?cvsroot=gcc&r1=1.176&r2=1.177

Comment 10 CVS Commits 2004-08-17 10:18:47 UTC
Subject: Bug 17019

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_4-branch
Changes by:	rearnsha@gcc.gnu.org	2004-08-17 10:18:32

Modified files:
	gcc            : ChangeLog 
	gcc/config/arm : arm.md 

Log message:
	From Daniel Bornstein  <danfuzz@milk.com>
	PR target/17019
	* arm.md (addsi3_cbranch_scratch): Correct case labels.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=2.2326.2.582&r2=2.2326.2.583
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/arm/arm.md.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.154&r2=1.154.4.1

Comment 11 CVS Commits 2004-08-17 11:32:30 UTC
Subject: Bug 17019

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	csl-arm-branch
Changes by:	rearnsha@gcc.gnu.org	2004-08-17 11:32:24

Modified files:
	gcc            : ChangeLog.csl-arm 
	gcc/config/arm : arm.md 

Log message:
	From Daniel Bornstein  <danfuzz@milk.com>
	PR target/17019
	* arm.md (addsi3_cbranch_scratch): Correct case labels.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.csl-arm.diff?cvsroot=gcc&only_with_tag=csl-arm-branch&r1=1.1.2.6&r2=1.1.2.7
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/arm/arm.md.diff?cvsroot=gcc&only_with_tag=csl-arm-branch&r1=1.145.2.24&r2=1.145.2.25

Comment 12 Richard Earnshaw 2004-08-17 11:33:42 UTC
I've applied the patch you proposed.