[Bug middle-end/66588] New: combine should try transforming if_then_else of zero_extends into zero_extend of if_then_else

[Bug middle-end/66588] New: combine should try transforming if_then_else of zero_extends into zero_extend of if_then_else

[ktkachov at gcc dot gnu.org]

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

            Bug ID: 66588
           Summary: combine should try transforming if_then_else of
                    zero_extends into zero_extend of if_then_else
           Product: gcc
           Version: 6.0
            Status: UNCONFIRMED
          Keywords: missed-optimization
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ktkachov at gcc dot gnu.org
  Target Milestone: ---
            Target: aarch64*

Consider the code:

unsigned long
foo (unsigned int a, unsigned int b, unsigned int c)
{
  return a ? b : c;
}

On aarch64 this generates:
foo:
        uxtw    x1, w1
        cmp     w0, wzr
        uxtw    x2, w2
        csel    x0, x2, x1, eq
        ret

where in fact it could generate:
        cmp      w0, #0
        csel    w0, w1, w2, ne
        ret

A write to the 32-bit w-register implicitly zero-extends the value up to the
full
64 bits of an x-register.

This is reflected in aarch64.md by the cmovsi_insn_uxtw pattern that matches:
(set (dest:DI)
     (zero_extend:DI
        (if_then_else:SI (cond) (src1:SI) (src2:SI))
     )
)

However, this doesn't get matched because combine instead tries to match
(set (dest:DI)
     (if_then_else:DI (cond)
                      (zero_extend:DI (src1:SI))
                      (zero_extend:DI (src2:SI))
     )
)

If I change the pattern to the second form then I get the desired codegen.
However, it seems that combine should really be trying that transformation by
itself.

[Bug middle-end/66588] combine should try transforming if_then_else of zero_extends into zero_extend of if_then_else

[ubizjak at gmail dot com]

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

--- Comment #1 from Uroš Bizjak <ubizjak at gmail dot com> ---
BTW: x86_64 is missing any form of zero-extended cmove.
>From gcc-bugs-return-489314-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Thu Jun 18 14:17:03 2015
Return-Path: <gcc-bugs-return-489314-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 60431 invoked by alias); 18 Jun 2015 14:17:02 -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 60370 invoked by uid 48); 18 Jun 2015 14:16:54 -0000
From: "ebotcazou at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug middle-end/66565] Problems and limitation GCC cost metrics and ways to improve the situation
Date: Thu, 18 Jun 2015 14:17: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: 6.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: ebotcazou 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: bug_status cf_reconfirmed_on cc everconfirmed
Message-ID: <bug-66565-4-BGm8Ciws8L@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-66565-4@http.gcc.gnu.org/bugzilla/>
References: <bug-66565-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-06/txt/msg01646.txt.bz2
Content-length: 2058

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-06-18
                 CC|                            |ebotcazou at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #1 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
>   The proliferation and misunderstanding of cost units is a side effect of
> this. So we really have to move to something like:
>
>   target_should_inline_call (<interesting call information from the pass>)
>   target_should_ifconvert (<gimple sequences for before/after
> transformation>)
>   target_combine_is_profitable_p (<before, after rtx>)
>   target_should_reconstruct_value_p (<candidate RTX to either build and keep
> in registers or reconstruct in loop>)
>   target_choose_preferred_addressing_mode (<list of candidate addressing
> modes for a MEM>)

So basically one hook per transformation?  And each back-end will have to
factorize the common processing?

>   Where we ask specific and meaningful questions of the target, rather than
> asking for a guess at costs and using that.
>   The difficult argument to make will be that we have to be radical about
> this and prepared to regress many targets (in the short term)
>   for the sake of ending up with a maintainable and understandable design
> for making these decisions.

We simply cannot do that, there are ~50 architectures in the compilers and this
will break them all except for a handful of them.  Generally speaking, you
cannot reimplement things from scratch in a 25 years old production compiler,
you will break far more things than you will ever be able to fix or improve.
Either you implement the new scheme in addition to the old one or you fix the
old one incrementally, that's pretty much the only practical alternative.

[Bug middle-end/66588] combine should try transforming if_then_else of zero_extends into zero_extend of if_then_else

[ubizjak at gmail dot com]

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

--- Comment #2 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Uroš Bizjak from comment #1)
> BTW: x86_64 is missing any form of zero-extended cmove.

... please see [1] how x86_64 implements it (*movsicc_noc_zext).

[1] https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01306.html
>From gcc-bugs-return-489349-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Thu Jun 18 20:51:24 2015
Return-Path: <gcc-bugs-return-489349-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 51430 invoked by alias); 18 Jun 2015 20:51:24 -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 51398 invoked by uid 48); 18 Jun 2015 20:51:20 -0000
From: "mikael at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug fortran/66578] [F2008] Invalid free on allocate(...,source=a(:)) in block
Date: Thu, 18 Jun 2015 20:51:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: fortran
X-Bugzilla-Version: 6.0
X-Bugzilla-Keywords: wrong-code
X-Bugzilla-Severity: normal
X-Bugzilla-Who: mikael at gcc dot gnu.org
X-Bugzilla-Status: ASSIGNED
X-Bugzilla-Resolution:
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: vehre at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-66578-4-75zapfdyZE@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-66578-4@http.gcc.gnu.org/bugzilla/>
References: <bug-66578-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-06/txt/msg01681.txt.bz2
Content-length: 826

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

--- Comment #8 from Mikael Morin <mikael at gcc dot gnu.org> ---
diff --git a/gcc/fortran/trans-array.c b/gcc/fortran/trans-array.c
index fece3ab..0b96de1 100644
--- a/gcc/fortran/trans-array.c
+++ b/gcc/fortran/trans-array.c
@@ -7079,7 +7077,7 @@ gfc_conv_expr_descriptor (gfc_se *se, gfc_expr *expr)
            {
              tmp = gfc_conv_array_lbound (desc, n);
              tmp = fold_build2_loc (input_location, MINUS_EXPR,
-                                    TREE_TYPE (base), tmp, loop.from[dim]);
+                                    TREE_TYPE (base), tmp, from);
              tmp = fold_build2_loc (input_location, MULT_EXPR,
                                     TREE_TYPE (base), tmp,
                                     gfc_conv_array_stride (desc, n));

[Bug middle-end/66588] combine should try transforming if_then_else of zero_extends into zero_extend of if_then_else

[ktkachov at gcc dot gnu.org]

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

--- Comment #3 from ktkachov at gcc dot gnu.org ---
(In reply to Uroš Bizjak from comment #2)
> (In reply to Uroš Bizjak from comment #1)
> > BTW: x86_64 is missing any form of zero-extended cmove.
> 
> ... please see [1] how x86_64 implements it (*movsicc_noc_zext).
> 
> [1] https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01306.html

Thanks. As I mentioned in the original post this approach does work.
I was just hoping to argue that combine should automatically try both the
(zero_extend (if_then_else a b c))
and
(if_then_else a (zero_extend b) (zero_extend c))

In fact, wouldn't transforming:
(if_then_else a (zero_extend b) (zero_extend c))
into
(zero_extend (if_then_else a b c))
be considered a simplification
suitable for simplify-rtx.c ?
>From gcc-bugs-return-489379-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Fri Jun 19 08:36:27 2015
Return-Path: <gcc-bugs-return-489379-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 94352 invoked by alias); 19 Jun 2015 08:36:27 -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 94300 invoked by uid 48); 19 Jun 2015 08:36:23 -0000
From: "mikael at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug fortran/66578] [F2008] Invalid free on allocate(...,source=a(:)) in block
Date: Fri, 19 Jun 2015 08:36:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: fortran
X-Bugzilla-Version: 6.0
X-Bugzilla-Keywords: wrong-code
X-Bugzilla-Severity: normal
X-Bugzilla-Who: mikael at gcc dot gnu.org
X-Bugzilla-Status: ASSIGNED
X-Bugzilla-Resolution:
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: vehre at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-66578-4-KlSPjALu8q@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-66578-4@http.gcc.gnu.org/bugzilla/>
References: <bug-66578-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-06/txt/msg01711.txt.bz2
Content-length: 487

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

--- Comment #10 from Mikael Morin <mikael at gcc dot gnu.org> ---
(In reply to Thomas Koenig from comment #9)
> Unfortunately, this does not appear to fix the bug (at least not completely).
> I still get an invalid free.

Indeed, unfortunately this:

(In reply to Mikael Morin from comment #7)
> Don't care about the scalarizer; generate a correct descriptor to begin
> with, and the scalarizer will follow (hopefully).

is not true.

[Bug middle-end/66588] combine should try transforming if_then_else of zero_extends into zero_extend of if_then_else

[segher at gcc dot gnu.org]

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

Segher Boessenkool <segher at gcc dot gnu.org> changed:

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

--- Comment #4 from Segher Boessenkool <segher at gcc dot gnu.org> ---
combine should not in general try multiple ways to write the same
thing; this will not scale.  In this case, I don't see that some
backends would really like to write it one way and some the other,
so we should just use one way.

There are no defined canonicalization rules for this, but there
are for the similar cases involving MULT, where the extends are
moved inwards as well.  This is most in line with how other extends
are treated as well.

We probably should document the canonicalization rules better;
current practice for backends is to just look at what combine does
and use that; not a great idea in my opinion.

[Bug middle-end/66588] combine should try transforming if_then_else of zero_extends into zero_extend of if_then_else

[ktkachov at gcc dot gnu.org]

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

--- Comment #5 from ktkachov at gcc dot gnu.org ---
(In reply to Segher Boessenkool from comment #4)
> combine should not in general try multiple ways to write the same
> thing; this will not scale.  In this case, I don't see that some
> backends would really like to write it one way and some the other,
> so we should just use one way.
> 
> There are no defined canonicalization rules for this, but there
> are for the similar cases involving MULT, where the extends are
> moved inwards as well.  This is most in line with how other extends
> are treated as well.
> 
> We probably should document the canonicalization rules better;
> current practice for backends is to just look at what combine does
> and use that; not a great idea in my opinion.

Ok, thanks. I've opened PR 66776 for the aarch64 MD changes.
Would you like to keep this open to track any documentation changes?
Or shall we close this off?

[Bug middle-end/66588] combine should try transforming if_then_else of zero_extends into zero_extend of if_then_else

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Andrew Pinski <pinskia@gcc.gnu.org>:

https://gcc.gnu.org/g:04a9209dc865dafe3c9615f5c868aa3fd89b96cf

commit r14-89-g04a9209dc865dafe3c9615f5c868aa3fd89b96cf
Author: Andrew Pinski <apinski@marvell.com>
Date:   Thu Apr 13 00:40:40 2023 +0000

    i386: Add new pattern for zero-extend cmov

    After a phiopt change, I got a failure of cmov9.c.
    The RTL IR has zero_extend on the outside of
    the if_then_else rather than on the side. Both
    ways are considered canonical as mentioned in
    PR 66588.

    This fixes the failure I got and also adds a testcase
    which fails before even my phiopt patch but will pass
    with this patch.

    OK? Bootstrapped and tested on x86_64-linux-gnu with
    no regressions.

    gcc/ChangeLog:

            * config/i386/i386.md (*movsicc_noc_zext_1): New pattern.

    gcc/testsuite/ChangeLog:

            * gcc.target/i386/cmov10.c: New test.
            * gcc.target/i386/cmov11.c: New test.