[Bug other/66112] New: __builtin_mul_overflow for int16_t emits poor code

[Bug other/66112] New: __builtin_mul_overflow for int16_t emits poor code

[gcc.hall at gmail dot com]

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

            Bug ID: 66112
           Summary: __builtin_mul_overflow for int16_t emits poor code
           Product: gcc
           Version: 5.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: other
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gcc.hall at gmail dot com
  Target Milestone: ---

I am using all the three signed integer overflow builtin's for the four 
types: int8_t, int16_t, int32_t and int64_t on Intel x64.  In all cases except
one this produces optimal code, the operation followed by "jo".  See the test
case below.
For the int32_t type, from  the last atoi down:

    call    atoi    #
    imull   %eax, %ebx  # b, tmp100
    jo  .L3 #,

However, for int16_t this produces much larger code that does a 32 bit multiply
and checks for overflow by hand:

    call    atoi    #
    movswl  %bx, %esi   # D.5222, D.5222
    cwtl
    imull   %esi, %eax  # D.5222, tmp109
    movl    %eax, %edx  # tmp109, tmp110
    movl    %eax, %ecx  # tmp109, tmp111
    sarl    $16, %edx   #, tmp110
    sarw    $15, %cx    #, tmp111
    cmpw    %dx, %cx    # tmp110, tmp111
    jne .L7 #,

Even though a simple "imulw" followed by "jo" would work.

-------------------------------------------------------------------
#include <stdio.h>
#include <inttypes.h>
int
main( int argc, const char *argv[] )
{
  int16_t result, a = (int16_t)atoi(argv[1]), b = (int16_t)atoi( argv[2] );
  if( __builtin_mul_overflow( a, b, &result ) )
    printf( "Overflow\n");
 else
    printf( "Product is %d\n", result );
}

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 35522
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35522&action=edit
gcc5-pr66112.patch

Supposedly just using SWI248 instead of SWI48 iterator should fix this, though
not sure about all the AMD scheduling stuff.

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[ubizjak at gmail dot com]

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

--- Comment #2 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Jakub Jelinek from comment #1)
> Created attachment 35522 [details]
> gcc5-pr66112.patch
> 
> Supposedly just using SWI248 instead of SWI48 iterator should fix this,
> though not sure about all the AMD scheduling stuff.

Just copy HImode patterns, as is already case with QI, SI/DImode (and we
tolerated it). We can macroize these patterns latter, if needed at all.

I wonder, why HImode patterns were left out in the first place ...
>From gcc-bugs-return-486063-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Tue May 12 08:51:14 2015
Return-Path: <gcc-bugs-return-486063-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 35577 invoked by alias); 12 May 2015 08:51:14 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 35543 invoked by uid 48); 12 May 2015 08:51:09 -0000
From: "rguenth at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug middle-end/66110] uint8_t memory access not optimized
Date: Tue, 12 May 2015 08:51:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: middle-end
X-Bugzilla-Version: 5.1.0
X-Bugzilla-Keywords: alias, missed-optimization
X-Bugzilla-Severity: normal
X-Bugzilla-Who: rguenth at gcc dot gnu.org
X-Bugzilla-Status: NEW
X-Bugzilla-Resolution:
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: keywords bug_status cf_reconfirmed_on cc component resolution everconfirmed
Message-ID: <bug-66110-4-cOHIAjb8Ee@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-66110-4@http.gcc.gnu.org/bugzilla/>
References: <bug-66110-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-05/txt/msg00903.txt.bz2
Content-length: 2815

https://gcc.gnu.org/bugzilla/show_bug.cgi?idf110

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |alias, missed-optimization
             Status|RESOLVED                    |NEW
   Last reconfirmed|                            |2015-05-12
                 CC|                            |rguenth at gcc dot gnu.org
          Component|tree-optimization           |middle-end
         Resolution|FIXED                       |---
     Ever confirmed|0                           |1

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
So what happens is that GCC sees

  _3 = p_2(D)->p1;
  _3->f2 = 9;
  _5 = p_2(D)->p1;
  _5->f2 = 10;

and to remove the first store it first has to prove that _3 and _5 are equal.
CSE cannot prove this because it thinks the store to _3->f2 can clobber the
value at p_2(D)->p1 (if p->p1 points to p).

get_alias_set (_3->f2) returns 0, the alias oracle has special code to also
consider the alias set of the base objects:

  /* Do type-based disambiguation.  */
  if (base1_alias_set != base2_alias_set
      && !alias_sets_conflict_p (base1_alias_set, base2_alias_set))
    return false;

but their alias sets happen to conflict because struct s2 alias set is a subset
of the struct s1 alias set (which is because alias set zero is a subset of
the struct s1 alias set...):

struct GTY(()) alias_set_entry_d {
  /* The alias set number, as stored in MEM_ALIAS_SET.  */
  alias_set_type alias_set;

  /* Nonzero if would have a child of zero: this effectively makes this
     alias set the same as alias set zero.  */
  int has_zero_child;

(I've always questioned this... - the code that looks at has_zero_child in
alias_set_subset_of / alias_sets_conflict_p).

Index: gcc/alias.c
==================================================================--- gcc/alias.c (revision 222996)
+++ gcc/alias.c (working copy)
@@ -470,15 +470,13 @@ alias_sets_conflict_p (alias_set_type se
   /* See if the first alias set is a subset of the second.  */
   ase = get_alias_set_entry (set1);
   if (ase != 0
-      && (ase->has_zero_child
-         || ase->children->get (set2)))
+      && ase->children->get (set2))
     return 1;

   /* Now do the same, but with the alias sets reversed.  */
   ase = get_alias_set_entry (set2);
   if (ase != 0
-      && (ase->has_zero_child
-         || ase->children->get (set1)))
+      && ase->children->get (set1))
     return 1;

   /* The two alias sets are distinct and neither one is the

fixes this.  I don't remember what broke (but I remember trying this for a few
times - maybe with also changing alias_set_subset_of which isn't that obvious).

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[gcc.hall at gmail dot com]

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

--- Comment #3 from Jeremy <gcc.hall at gmail dot com> ---
Related FYI,
Few instructions on ARM set the overflow flag.  Two that do are 32-bit add and
subtract.  For these two, GCC could just emit "adds" followed by "bvs" 
Instead it produces:-

    bl  atoi    @
    add r1, r4, r0  @ tmp121, a, b
    cmp r0, #0  @ b,
    blt .L4 @,
    cmp r1, r4  @ tmp121, a
    bge .L5 @,
    b   .L3 @
.L4:
    cmp r1, r4  @ tmp121, a
    ble .L5 @,

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #35522|0                           |1
        is obsolete|                            |
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2015-05-12
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 35523
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35523&action=edit
gcc6-pr66112-1.patch

Generic fix, to improve minimum precision computation for unsigned values.

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 35524
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35524&action=edit
gcc6-pr66112-2.patch

And i386 mulvhi4 and umulvhi4 support.  For umulvhi4, I haven't found
corresponding i386.md instruction that would emit mul{w}, so just changed SWI48
to SWI248 iterator in that case.

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
As for ARM, please file a separate enhancement request.  The generic code has
the means for backends to provide better patterns, but arm doesn't use them
(only i386.md uses them right now), so arm maintainers should add those if
needed.

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[gcc.hall at gmail dot com]

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

--- Comment #7 from Jeremy <gcc.hall at gmail dot com> ---
Comment on attachment 35522
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35522
gcc5-pr66112.patch

Done, PR66120

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[ubizjak at gmail dot com]

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

--- Comment #8 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Jakub Jelinek from comment #5)
> Created attachment 35524 [details]
> gcc6-pr66112-2.patch
> 
> And i386 mulvhi4 and umulvhi4 support.  For umulvhi4, I haven't found
> corresponding i386.md instruction that would emit mul{w}, so just changed
> SWI48 to SWI248 iterator in that case.

No, mulw outputs to %ax:%dx pair of HImode registers! Please see [1]

[1] http://x86.renejeschke.de/html/file_module_x86_id_210.html
>From gcc-bugs-return-486092-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Tue May 12 10:24:15 2015
Return-Path: <gcc-bugs-return-486092-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 38607 invoked by alias); 12 May 2015 10:24:14 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 38542 invoked by uid 48); 12 May 2015 10:24:10 -0000
From: "rguenth at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code
Date: Tue, 12 May 2015 10:24:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: tree-optimization
X-Bugzilla-Version: 5.1.1
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: rguenth at gcc dot gnu.org
X-Bugzilla-Status: NEW
X-Bugzilla-Resolution:
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 5.2
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-66119-4-eQfKd87hd6@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-66119-4@http.gcc.gnu.org/bugzilla/>
References: <bug-66119-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-05/txt/msg00932.txt.bz2
Content-length: 617

https://gcc.gnu.org/bugzilla/show_bug.cgi?idf119

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
DEFPARAM (PARAM_SRA_MAX_SCALARIZATION_SIZE_SPEED,
          "sra-max-scalarization-size-Ospeed",
          "Maximum size, in storage units,

storage units!  But the value seems to be in bits?  It gets used as

            if (tree_to_uhwi (TYPE_SIZE (TREE_TYPE (var)))
                <= max_scalarization_size)

max_scalarization_size is 384, the aggregate size is 512.

Looks like get_move_ratio returns different things at SRA time (if I re-call
it)
and the time it gets invoked in toplev.c.

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Wed May 13 08:07:58 2015
New Revision: 223115

URL: https://gcc.gnu.org/viewcvs?rev=223115&root=gcc&view=rev
Log:
        PR target/66112
        * internal-fn.c (get_min_precision): Use UNSIGNED instead of
        SIGNED to get precision of non-negative value.

        * gcc.target/i386/pr66112-1.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/i386/pr66112-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/internal-fn.c
    trunk/gcc/testsuite/ChangeLog

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Wed May 13 08:09:01 2015
New Revision: 223116

URL: https://gcc.gnu.org/viewcvs?rev=223116&root=gcc&view=rev
Log:
        PR target/66112
        * config/i386/i386.md (mulv<mode>4, umulv<mode>4, *umulv<mode>4):
        Use SWI248 iterator instead of SWI.
        (*mulv<mode>4_1): Use SWI48 instead of SWI.  Simplify output template.
        Use eq_attr "alternative" "0" instead of match_test in
        length_immediate attribute computation.
        (*mulvhi4, *mulvhi4_1): New define_insns.

        * gcc.target/i386/pr66112-2.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/i386/pr66112-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.md
    trunk/gcc/testsuite/ChangeLog

[Bug target/66112] __builtin_mul_overflow for int16_t emits poor code

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk.  Not a regression, so probably not suitable for
backporting.