Bug 87248 - [6/7 Regression] Bad code for masked operations involving signed ints
Summary: [6/7 Regression] Bad code for masked operations involving signed ints
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 8.2.1
: P3 normal
Target Milestone: 6.5
Assignee: Jakub Jelinek
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2018-09-07 08:09 UTC by David Brown
Modified: 2018-10-12 22:21 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-09-07 00:00:00


Attachments
gcc9-pr87248.patch (1015 bytes, patch)
2018-09-07 23:23 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Brown 2018-09-07 08:09:31 UTC
I am always wary of saying there might be a compiler bug - usually it is a bug in the user code.  But this time I am very suspicious.  The example here comes from a discussion in the comp.lang.c Usenet group.

Here is the code I have been testing:


unsigned char foo_u(unsigned int v) {
    return (v & 0x7f) | (v & ~0x7f ? 0x80 : 0);
}

unsigned char foo_s(int v) {
    return (v & 0x7f) | (v & ~0x7f ? 0x80 : 0);
}

unsigned char bar_s(int v) {
    int x = v & ~0x7f;
    return (v & 0x7f) | (x ? 0x80 : 0);
}


int test_u_1(void) {
    return foo_u(0x01);        // Expect 0x01 = 1
}

int test_u_2(void) {
    return foo_u(0x1001);    // Expect 0x81 = 129
}

int test_s_1(void) {
    return foo_s(0x01);        // Expect 0x01 = 1
}

int test_s_2(void) {
    return foo_s(0x1001);    // Expect 0x81 = 129
}


The assembly code generated for this is:

foo_u(unsigned int):
        mov     eax, edi
        mov     edx, edi
        or      edx, -128
        and     eax, 127
        and     edi, -128
        cmovne  eax, edx
        ret
foo_s(int):
        mov     eax, edi
        ret
bar_s(int):
        mov     eax, edi
        mov     edx, edi
        or      edx, -128
        and     eax, 127
        and     edi, 127
        cmovne  eax, edx
        ret
test_u_1():
        mov     eax, 1
        ret
test_u_2():
        mov     eax, 129
        ret
test_s_1():
        mov     eax, 1
        ret
test_s_2():
        mov     eax, 1
        ret



When the code uses "int" rather than "unsigned int", in "foo_s", the compiler thinks the function can be optimised to a simple byte extraction.  I cannot see how that interpretation is valid.  And gcc cannot see it either, if there is an intermediate variable (in "bar_s").  (The type of the intermediate "x" in "bar_s" does not matter - "int", "unsigned int", "auto", "__auto_type", const or not).

This effect is happening in the front-end.  It is independent of optimisation levels (-O gives the same results), the target processor (I tried a half-dozen targets), the compiler version (from gcc 4.4 upwards - gcc 4.1 gives the same code for foo_s as foo_u.  I haven't tried gcc 4.2 or 4.3).  There are minor variations in the generated code (such as the use of conditional move instructions) between different versions - those don't affect the results.  The "-fwrapv" flag has no effect either, nor does the choice of C or C++.

The results from the compiler evaluations in the "test_" functions shows that it this happens in the compiler analysis - it is not a code generation issue.

(<https://godbolt.org> is great for this kind of testing!)

(This bug report was first sent to the mailing list <https://gcc.gnu.org/ml/gcc/2018-09/msg00028.html> - the report here has a few minor corrections.)
Comment 1 Jakub Jelinek 2018-09-07 17:47:41 UTC
      /* A & N ? N : 0 is simply A & N if N is a power of two.  This
         is probably obsolete because the first operand should be a
         truth value (that's why we have the two cases above), but let's
         leave it in until we can confirm this for all front-ends.  */
      if (integer_zerop (op2)
          && TREE_CODE (arg0) == NE_EXPR
          && integer_zerop (TREE_OPERAND (arg0, 1))
          && integer_pow2p (arg1) 
          && TREE_CODE (TREE_OPERAND (arg0, 0)) == BIT_AND_EXPR
          && operand_equal_p (TREE_OPERAND (TREE_OPERAND (arg0, 0), 1),
                              arg1, OEP_ONLY_CONST))
        return pedantic_non_lvalue_loc (loc,
                                    fold_convert_loc (loc, type,
                                                      TREE_OPERAND (arg0, 0)));
is the problem, while arg1 (8-bit) is integer_pow2p, TREE_OPERAND (TREE_OPERAND (arg0, 0), 1)) is equal to it (-128), but is not a pow2p.
Comment 2 Jakub Jelinek 2018-09-07 23:23:57 UTC
Created attachment 44671 [details]
gcc9-pr87248.patch

Untested fix.
Comment 3 Jakub Jelinek 2018-09-12 09:19:27 UTC
Author: jakub
Date: Wed Sep 12 09:18:55 2018
New Revision: 264230

URL: https://gcc.gnu.org/viewcvs?rev=264230&root=gcc&view=rev
Log:
	PR middle-end/87248
	* fold-const.c (fold_ternary_loc) <case COND_EXPR>: Verify also that
	BIT_AND_EXPR's second operand is a power of two.  Formatting fix.

	* c-c++-common/torture/pr87248.c: New test.

Added:
    trunk/gcc/testsuite/c-c++-common/torture/pr87248.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/fold-const.c
    trunk/gcc/testsuite/ChangeLog
Comment 4 Jakub Jelinek 2018-09-12 09:21:35 UTC
Author: jakub
Date: Wed Sep 12 09:21:03 2018
New Revision: 264231

URL: https://gcc.gnu.org/viewcvs?rev=264231&root=gcc&view=rev
Log:
	PR middle-end/87248
	* fold-const.c (fold_ternary_loc) <case COND_EXPR>: Verify also that
	BIT_AND_EXPR's second operand is a power of two.  Formatting fix.

	* c-c++-common/torture/pr87248.c: New test.

Added:
    branches/gcc-8-branch/gcc/testsuite/c-c++-common/torture/pr87248.c
Modified:
    branches/gcc-8-branch/gcc/ChangeLog
    branches/gcc-8-branch/gcc/fold-const.c
    branches/gcc-8-branch/gcc/testsuite/ChangeLog
Comment 5 Jakub Jelinek 2018-09-17 08:21:01 UTC
Fixed for 8.3+ so far.
Comment 6 Jakub Jelinek 2018-10-12 14:55:06 UTC
Author: jakub
Date: Fri Oct 12 14:54:34 2018
New Revision: 265111

URL: https://gcc.gnu.org/viewcvs?rev=265111&root=gcc&view=rev
Log:
	Backported from mainline
	2018-09-12  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/87248
	* fold-const.c (fold_ternary_loc) <case COND_EXPR>: Verify also that
	BIT_AND_EXPR's second operand is a power of two.  Formatting fix.

	* c-c++-common/torture/pr87248.c: New test.

Added:
    branches/gcc-7-branch/gcc/testsuite/c-c++-common/torture/pr87248.c
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/fold-const.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
Comment 7 Jakub Jelinek 2018-10-12 17:33:56 UTC
Author: jakub
Date: Fri Oct 12 17:33:25 2018
New Revision: 265121

URL: https://gcc.gnu.org/viewcvs?rev=265121&root=gcc&view=rev
Log:
	Backported from mainline
	2018-09-12  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/87248
	* fold-const.c (fold_ternary_loc) <case COND_EXPR>: Verify also that
	BIT_AND_EXPR's second operand is a power of two.  Formatting fix.

	* c-c++-common/torture/pr87248.c: New test.

Added:
    branches/gcc-6-branch/gcc/testsuite/c-c++-common/torture/pr87248.c
Modified:
    branches/gcc-6-branch/gcc/ChangeLog
    branches/gcc-6-branch/gcc/fold-const.c
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
Comment 8 Jakub Jelinek 2018-10-12 22:21:18 UTC
Fixed.