bad switch statement in md code for addsi3_cbranch_scratch (original) (raw)

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🔞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 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 12 Richard Earnshaw 2004-08-17 11:33:42 UTC

I've applied the patch you proposed.