Forum Discussion

Altera_Forum's avatar
Altera_Forum
Icon for Honored Contributor rankHonored Contributor
14 years ago

gcc generates incorrect code

Hi,

It appears that there is a bug in this version of the nios2 compiler at higher optimisation levels.

$ nios2-elf-gcc --version

nios2-elf-gcc (Altera 11.1sp2 Build 259) 4.1.2

Copyright (C) 2006 Free Software Foundation, Inc.

This is free software; see the source for copying conditions. There is NO

warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I can isolate to a small source code as follows.

# include <stdint.h>

uint32_t makeMask ( const uint32_t l )

{

uint32_t mask;

if ( l < 32u ) {

static const uint32_t one = 1u;

const unsigned idx = 32u - l;

uint32_t mask = one;

mask <<= idx;

mask -= one;

}

else {

mask = 31u;

}

return mask;

}

Running gcc with these options (highest level of optimisation and producing an assembler listing) appears to indicate that there is a bug in the nios2 compiler.

nios2-elf-gcc -c -O3 -S test.c

.file "test.c"

.section .text

.align 2

.global makeMask

.type makeMask, @function

makeMask:

movi r2, 31

ret

.size makeMask, .-makeMask

.ident "GCC: (Altera 11.1sp2 Build 259) 4.1.2"

3 Replies

  • Altera_Forum's avatar
    Altera_Forum
    Icon for Honored Contributor rankHonored Contributor
    14 years ago

    The problem is with your code, not the compiler.

    You define mask in two different scopes. All the bit manipulation you do in the first branch of the if-statement is on your second definition of mask. Unfortunately you don't return that one, but the one defined in the outer scope.

    So basically you're saying to the compiler, return an uninitialised number or 31. In this case the result is undefined, so the compiler is probably free to do what it likes. It likes 31.

    To fix your code, remove type definition from the first assignment to mask.
  • Altera_Forum's avatar
    Altera_Forum
    Icon for Honored Contributor rankHonored Contributor
    14 years ago

    Thanks dabuk for your correction. You are absolutely correct.

    BTW, while consuming BW, there was in fact a funny behaviour remaining in this code after it is corrected as debuk suggests (which in fact is the one I was originally pursuing). Do you see it? It resulted from a more subtle programming error (unfortunately still my fault STS). The following is a very bad idea in portable C code.

    uint32_t mask;

    if ( l < 32u ) {

    static const uint32_t one = 1u;

    const unsigned idx = 32u - l;

    uint32_t mask = one;

    mask <<= idx;

    mask -= one;

    }

    This one is correct:

    uint32_t mask;

    if ( l > 0 && l < 32u ) {

    static const uint32_t one = 1u;

    const unsigned idx = 32u - l;

    uint32_t mask = one;

    mask <<= idx;

    mask -= one;

    }

    The problem in the first version is that the C standard says that the result of a left shift is undefined if we specify a shift of more bits than are in the word, and 32 is in fact too many. The nios2 RTL for the SLL instruction is as follows.

    rC &#8592; rA << (rB4..0)

    Notice that it uses only 5 bits from the register, shoot self in foot.

    Ouch
  • Altera_Forum's avatar
    Altera_Forum
    Icon for Honored Contributor rankHonored Contributor
    14 years ago

    You'll also find that the x86 cpu ignores all the high bits of a shift count.

    Oh definitions like: 

    static const uint32_t one = 1u;
    are just completely pointless.

    It is much clearer if you just write: 

    uint32_t makeMask(uint32_t count)
{
    return count >= 31 ? 0xffffffffu : (1u << count) - 1;
}
    All the temporary variables just make it harder to read.

    (That isn't quite your function, but is closer to the one you wanted!)