[Bug c/52708] New: suboptimal code with __builtin_constant_p

[Bug c/52708] New: suboptimal code with __builtin_constant_p

[tijl at coosemans dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52708

             Bug #: 52708
           Summary: suboptimal code with __builtin_constant_p
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: tijl@coosemans.org


Consider the following code:

--------------------
#include <stdio.h>

int bar(void) __attribute__((__const__));

int main( int argc, char **argv ) {
    int res;

    res = (__builtin_constant_p(bar()) ? 0 : 1);
    printf( "%d\n", res );

    res = (__builtin_constant_p(bar()) ? 0 : bar());
    printf( "%d\n", res );

    return(0);
}
--------------------

It produces the following i386 asm output for main (gcc47 -O3 -S):
(gcc47 (FreeBSD Ports Collection) 4.7.0 20120128 (experimental))

--------------------
main:
    pushl    %ebp
    movl    %esp, %ebp
    pushl    %ebx
    andl    $-16, %esp
    subl    $16, %esp
    call    foo        ; foo called before first printf
    movl    $1, 4(%esp)
    movl    $.LC0, (%esp)
    movl    %eax, %ebx    ; return value needs to be saved
    call    printf
    movl    %ebx, 4(%esp)    ; foo should have been called here
    movl    $.LC0, (%esp)
    call    printf
    xorl    %eax, %eax
    movl    -4(%ebp), %ebx
    leave
    ret
--------------------

When the __const__ attribute is removed, gcc does produce the right code.

I've worked out a more elaborate example below that shows that gcc can emit
code to evaluate the argument of __builtin_constant_p(). The foo call above
comes from __builtin_constant_p(foo()). I expected __builtin_constant_p() to be
a compile time constant that never produces any code.

In the case below there are two functions foo() and bar(). They are identical
except for a __const__ attribute. Both call a function increment() that simply
counts how many times it's called and then they return their argument. main()
has three tests with __builtin_constant_p().

--------------------
#include <stdio.h>

int count;

void increment(void);
int foo(int a) __attribute__((__const__));
int bar(int a);

void increment(void) {
    count++;
}

int foo(int a) {
    increment();
    return(a);
}

int bar(int a) {
    increment();
    return(a);
}

int main( int argc, char **argv ) {
    int res;

    /* without const attribute */
    count = 0;
    res = (__builtin_constant_p(bar(argc)) ? 0 : bar(argc));
    printf( "count(%d) res(%d)\n", count, res );

    /* with const attribute (bar) */
    count = 0;
    res = (__builtin_constant_p(foo(argc)) ? 0 : bar(argc));
    printf( "count(%d) res(%d)\n", count, res );

    /* with const attribute (foo) */
    count = 0;
    res = (__builtin_constant_p(foo(argc)) ? 0 : foo(argc));
    printf( "count(%d) res(%d)\n", count, res );

    return(0);
}
--------------------

Outputs of this program with various optimisation levels:

% gcc47 -O0 -o test test.c
% ./test
count(1) res(1)
count(1) res(1)
count(1) res(1)
% gcc47 -O1 -o test test.c
% ./test
count(1) res(1)
count(2) res(1)
count(0) res(1)
% gcc47 -O2 -o test test.c
% ./test
count(1) res(1)
count(2) res(1)
count(2) res(1)
% gcc47 -O3 -o test test.c
% ./test
count(1) res(1)
count(2) res(1)
count(2) res(1)

Possible outcomes:
1) count(1) res(1): ok: __builtin_constant_p returned false, foo or bar called
once.
2) count(2) res(1): not ok: code was emitted for __builtin_constant_p(foo())
resulting in a call to increment(). __builtin_constant_p returned false, then
foo or bar is called which call increment() a second time.
3) count(0) res(1): not really ok: __builtin_constant_p returned false, foo
called once, but because foo is declared const the call can be eliminated if
the compiler can reuse the result of a previous call, so count stays zero.
However, there's only one call to foo() in the above code.

Note that in this test program foo() isn't really const. If foo is truly const
I could not get gcc to produce wrong code, only suboptimal code as in the first
test program. Sometimes it is useful to let the compiler treat a function as
const even if it has side effects though, such as math functions that can
produce FPU exceptions. The current behaviour of __builtin_constant_p defeats
this (i.e. there could be two exceptions instead of (at most) one).

As a final note, replacing ?: with __builtin_choose_expr results in errors
like:
bug.c:33: error: first argument to '__builtin_choose_expr' not a constant
bug.c:38: error: first argument to '__builtin_choose_expr' not a constant

I expected __builtin_constant_p(expr) to be a compile time constant expression
even if expr isn't.

[Bug c/52708] suboptimal code with __builtin_constant_p

[tijl at coosemans dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52708

--- Comment #1 from Tijl Coosemans <tijl at coosemans dot org> 2012-03-25 20:09:34 UTC ---
(In reply to comment #0)
> Consider the following code:
> 
> --------------------
> #include <stdio.h>
> 
> int bar(void) __attribute__((__const__));
> 
> int main( int argc, char **argv ) {
>     int res;
> 
>     res = (__builtin_constant_p(bar()) ? 0 : 1);
>     printf( "%d\n", res );
> 
>     res = (__builtin_constant_p(bar()) ? 0 : bar());
>     printf( "%d\n", res );
> 
>     return(0);
> }
> --------------------

"bar" should have been "foo" here, to match the surrounding text.

[Bug c/52708] suboptimal code with __builtin_constant_p

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52708

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2012-03-26
     Ever Confirmed|0                           |1

--- Comment #2 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-03-26 08:17:39 UTC ---
Hm.  We delay evaluating __builtin_constant_p to make it possible for inlining
to lead to simplifications that result in a constant.  We could of course
evaluate early if there are any side-effects in its argument, but that would
change outcome in existing code bases.

What do you think specifies the behavior you are expecting?  Nothing
explicitely
says that the argument is not evaluated or that its side-effects are discarded.

[Bug c/52708] suboptimal code with __builtin_constant_p

[tijl at coosemans dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52708

--- Comment #3 from Tijl Coosemans <tijl at coosemans dot org> 2012-03-27 11:33:41 UTC ---
(In reply to comment #2)
> Hm.  We delay evaluating __builtin_constant_p to make it possible for inlining
> to lead to simplifications that result in a constant.  We could of course
> evaluate early if there are any side-effects in its argument, but that would
> change outcome in existing code bases.
> 
> What do you think specifies the behavior you are expecting?  Nothing
> explicitely says that the argument is not evaluated or that its
> side-effects are discarded.

Either the compiler always emits code to evaluate the argument or it never
emits code, but not something in between. That's just arbitrary.

I think it should never emit code because __builtin_constant_p is just a query
to the compiler: "Can you reduce this expression to a constant at compile time
or not?" It's not a request to emit code for the given expression.

It's ok for the compiler to inline the function and see if that can be reduced
to a constant, but that should not result in any code being emitted. If it does
it causes the compiler to make bad decisions such as in the first test program
where it needs an extra register (%ebx) that needs to be saved on the stack.

[Bug c/52708] suboptimal code with __builtin_constant_p

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52708

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |pinskia at gcc dot gnu.org

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 31190
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31190&action=edit
Patch which I think fixes this issue

I think this is a valid complaint.  I attached a patch which is against 4.7.0
which should fix this issue.  The problem is PRE tries to pull out the
__builtin_constant_p call and then optimizes based on that.

[Bug tree-optimization/52708] suboptimal code with __builtin_constant_p

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52708

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

[Bug tree-optimization/52708] suboptimal code with __builtin_constant_p

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52708

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|pinskia at gcc dot gnu.org         |unassigned at gcc dot gnu.org
             Status|ASSIGNED                    |NEW