[Bug rtl-optimization/94613] New: combine: Wrong code due to splitting a simplified IF_THEN_ELSE

[Bug rtl-optimization/94613] New: combine: Wrong code due to splitting a simplified IF_THEN_ELSE

[krebbel at gcc dot gnu.org]

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

            Bug ID: 94613
           Summary: combine: Wrong code due to splitting a simplified
                    IF_THEN_ELSE
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: krebbel at gcc dot gnu.org
  Target Milestone: ---

Created attachment 48284
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48284&action=edit
autoreduced testcase

When compiling the attached testcase for IBM Z GCC optimizes an OR operation
away during combine leading to wrong code being generated:

cc1plus -O3 -march=z14 t.ii -quiet -o t.s

try_combine is invoked for the following 3 insns:

i1: 28: r111 = r110 | r158
i2: 31: r115 = r111 - r94
i3: 36: r122 = (r110 == 0 ? r115 : r177)

i3 becomes the following after inserting i1 and i2

36: r122 = (r110 == 0 ? r110 | r158 - r94 : r177)

simplification omits ORing r110 since it is 0 anyway:

36: r122 = (r110 == 0 ? r158 - r94 : r177)

after not being able to recognize the insn it gets split into parts again:

28: deleted
31: r115 = r158 - r94
36: r122 = (r110 == 0 ? r115 : r177)

[Bug rtl-optimization/94613] combine: Wrong code due to splitting a simplified IF_THEN_ELSE

[krebbel at gcc dot gnu.org]

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

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
           Priority|P3                          |P1

[Bug rtl-optimization/94613] combine: Wrong code due to splitting a simplified IF_THEN_ELSE

[krebbel at gcc dot gnu.org]

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

--- Comment #1 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
If we have to assume that we already applied simplifications on the THEN or
ELSE branches it doesn't appear to be correct to look for split points inside
an IF_THEN_ELSE expression anymore.

This patch fixes the testcase for me:

index cff76cd3303..8f90a354977 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -5387,8 +5387,12 @@ find_split_point (rtx *loc, rtx_insn *insn, bool
set_src)
   /* Otherwise, select our actions depending on our rtx class.  */
   switch (GET_RTX_CLASS (code))
     {
-    case RTX_BITFIELD_OPS:             /* This is ZERO_EXTRACT and
SIGN_EXTRACT.  */
     case RTX_TERNARY:
+      /* Don't split inside then/else branches of an IF_THEN_ELSE.
+        Simplification might have changed the expressions based on
+        the condition.  */
+      return 0;
+    case RTX_BITFIELD_OPS:             /* This is ZERO_EXTRACT and
SIGN_EXTRACT.  */
       split = find_split_point (&XEXP (x, 2), insn, false);
       if (split)
        return split;

[Bug rtl-optimization/94613] combine: Wrong code due to splitting a simplified IF_THEN_ELSE

[krebbel at gcc dot gnu.org]

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

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.0

[Bug rtl-optimization/94613] combine: Wrong code due to splitting a simplified IF_THEN_ELSE

[krebbel at gcc dot gnu.org]

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

--- Comment #2 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
-fno-ipa-sra is required to get the same results as in the first comment. The
full command line then is:

c1plus -O3 -march=z14 t.ii -quiet -o t.s -fno-ipa-sra

[Bug rtl-optimization/94613] combine: Wrong code due to splitting a simplified IF_THEN_ELSE

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Why is it not correct to split the insn the way you describe?  I see nothing
wrong with that - the use of r115 is still under r110 == 0.  Is the issue
that r115 is re-used and r115 has more than a single use?

[Bug rtl-optimization/94613] combine: Wrong code due to splitting a simplified IF_THEN_ELSE

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
You might be able to turn this into a RTL testcase with a C driver to make it
suitable for a dg-do run testcase.  There's a combine testcase at
gcc.dg/rtl/aarch64/asr_div1.c you could look at (just not dg-do run)

[Bug rtl-optimization/94613] combine: Wrong code due to splitting a simplified IF_THEN_ELSE

[krebbel at gcc dot gnu.org]

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

--- Comment #5 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> Why is it not correct to split the insn the way you describe?  I see nothing
> wrong with that - the use of r115 is still under r110 == 0.  Is the issue
> that r115 is re-used and r115 has more than a single use?

Yes that's what it appeared to me in the original testcase but I have just
noticed that auto-reduction broke it.  In the reduced testcase r115 dies in
INSN 36 so it isn't useful right now. I agree that the splitting is legal in
that case.

[Bug target/94613] Wrong code generated for vec_sel builtin

[krebbel at gcc dot gnu.org]

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

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P1                          |P2
            Summary|combine: Wrong code due to  |Wrong code generated for
                   |splitting a simplified      |vec_sel builtin
                   |IF_THEN_ELSE                |
          Component|rtl-optimization            |target
           Severity|critical                    |normal

[Bug target/94613] S/390: Wrong code generated for vec_sel builtin

[krebbel at gcc dot gnu.org]

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

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P2                          |P3
           Assignee|unassigned at gcc dot gnu.org      |krebbel at gcc dot gnu.org

--- Comment #6 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
My debugging went into the wrong direction. The problem arises from wrong RTL
being generated in the backend for the vec_sel builtin.

[Bug target/94613] S/390: Wrong code generated for vec_sel builtin

[segher at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|s390x                       |s390x powerpc*-*-*
   Last reconfirmed|                            |2020-04-16
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #7 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Yes.  The rs6000 code is bad as well:

(define_insn "*altivec_vsel<mode>"
  [(set (match_operand:VM 0 "altivec_register_operand" "=v")
        (if_then_else:VM
         (ne:CC (match_operand:VM 1 "altivec_register_operand" "v")
                (match_operand:VM 4 "zero_constant" ""))
         (match_operand:VM 2 "altivec_register_operand" "v")
         (match_operand:VM 3 "altivec_register_operand" "v")))]
  "VECTOR_MEM_ALTIVEC_P (<MODE>mode)"
  "vsel %0,%3,%2,%1"
  [(set_attr "type" "vecmove")])

That is not what the insn does (the compare should be *bitwise*: it does

  op0 := (op2 & ~op1) | (op3 & op1)

This can be written a few different ways in RTL; we'll have to find out
what works best.

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Andreas Krebbel <krebbel@gcc.gnu.org>:

https://gcc.gnu.org/g:2930bb321794c241d8df5591a5bf447bf89c6e82

commit r10-7827-g2930bb321794c241d8df5591a5bf447bf89c6e82
Author: Andreas Krebbel <krebbel@linux.ibm.com>
Date:   Mon Apr 20 19:36:33 2020 +0200

    PR94613: Fix vec_sel builtin for IBM Z

    The vsel instruction is a bit-wise select instruction.  Using an
    IF_THEN_ELSE to express it in RTL is wrong and leads to wrong code being
    generated in the combine pass.

    With the patch the pattern is written using bit operations.  However,
    I've just noticed that the manual still demands a fixed point mode for
    AND/IOR and friends although several targets emit bit ops on floating
    point vectors (including i386, Power, and s390). So I assume this is a
    safe thing to do?!

    gcc/ChangeLog:

    2020-04-20  Andreas Krebbel  <krebbel@linux.ibm.com>

            PR target/94613
            * config/s390/s390-builtin-types.def: Add 3 new function modes.
            * config/s390/s390-builtins.def: Add mode dependent low-level
            builtin and map the overloaded builtins to these.
            * config/s390/vx-builtins.md ("vec_selV_HW"): Rename to ...
            ("vsel<V_HW"): ... this and rewrite the pattern with bitops.

    gcc/testsuite/ChangeLog:

    2020-04-20  Andreas Krebbel  <krebbel@linux.ibm.com>

            PR target/94613
            * gcc.target/s390/zvector/pr94613.c: New test.
            * gcc.target/s390/zvector/vec_sel-1.c: New test.

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Andreas Krebbel
<krebbel@gcc.gnu.org>:

https://gcc.gnu.org/g:92ee7d437a98a0f9e63549b4aa83af87382821cf

commit r9-8562-g92ee7d437a98a0f9e63549b4aa83af87382821cf
Author: Andreas Krebbel <krebbel@linux.ibm.com>
Date:   Mon Apr 20 19:36:33 2020 +0200

    PR94613: Fix vec_sel builtin for IBM Z

    The vsel instruction is a bit-wise select instruction.  Using an
    IF_THEN_ELSE to express it in RTL is wrong and leads to wrong code being
    generated in the combine pass.

    With the patch the pattern is written using bit operations.  However,
    I've just noticed that the manual still demands a fixed point mode for
    AND/IOR and friends although several targets emit bit ops on floating
    point vectors (including i386, Power, and s390). So I assume this is a
    safe thing to do?!

    gcc/ChangeLog:

    2020-05-04  Andreas Krebbel  <krebbel@linux.ibm.com>

            Backport from mainline
            2020-04-20  Andreas Krebbel  <krebbel@linux.ibm.com>

            PR target/94613
            * config/s390/s390-builtin-types.def: Add 3 new function modes.
            * config/s390/s390-builtins.def: Add mode dependent low-level
            builtin and map the overloaded builtins to these.
            * config/s390/vx-builtins.md ("vec_selV_HW"): Rename to ...
            ("vsel<V_HW"): ... this and rewrite the pattern with bitops.

    gcc/testsuite/ChangeLog:

    2020-05-04  Andreas Krebbel  <krebbel@linux.ibm.com>

            Backport from mainline
            2020-04-20  Andreas Krebbel  <krebbel@linux.ibm.com>

            PR target/94613
            * gcc.target/s390/zvector/pr94613.c: New test.
            * gcc.target/s390/zvector/vec_sel-1.c: New test.

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-8 branch has been updated by Andreas Krebbel
<krebbel@gcc.gnu.org>:

https://gcc.gnu.org/g:cf16ecdd897a73a1651aba50533c5a6fd73e842c

commit r8-10232-gcf16ecdd897a73a1651aba50533c5a6fd73e842c
Author: Andreas Krebbel <krebbel@linux.ibm.com>
Date:   Mon May 4 10:43:02 2020 +0200

    PR94613: Fix vec_sel builtin for IBM Z

    The vsel instruction is a bit-wise select instruction.  Using an
    IF_THEN_ELSE to express it in RTL is wrong and leads to wrong code being
    generated in the combine pass.

    With the patch the pattern is written using bit operations.  However,
    I've just noticed that the manual still demands a fixed point mode for
    AND/IOR and friends although several targets emit bit ops on floating
    point vectors (including i386, Power, and s390). So I assume this is a
    safe thing to do?!

    gcc/ChangeLog:

    2020-05-04  Andreas Krebbel  <krebbel@linux.ibm.com>

            Backport from mainline
            2020-04-20  Andreas Krebbel  <krebbel@linux.ibm.com>

            PR target/94613
            * config/s390/s390-builtin-types.def: Add 3 new function modes.
            * config/s390/s390-builtins.def: Add mode dependent low-level
            builtin and map the overloaded builtins to these.
            * config/s390/vx-builtins.md ("vec_selV_HW"): Rename to ...
            ("vsel<V_HW"): ... this and rewrite the pattern with bitops.

    gcc/testsuite/ChangeLog:

    2020-05-04  Andreas Krebbel  <krebbel@linux.ibm.com>

            Backport from mainline
            2020-04-20  Andreas Krebbel  <krebbel@linux.ibm.com>

            PR target/94613
            * gcc.target/s390/zvector/pr94613.c: New test.
            * gcc.target/s390/zvector/vec_sel-1.c: New test.

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.0                        |10.2

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.1 has been released.

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.2                        |10.3

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10.2 is released, adjusting target milestone.

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.3                        |10.4

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10.3 is being released, retargeting bugs to GCC 10.4.

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[luoxhu at gcc dot gnu.org]

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

luoxhu at gcc dot gnu.org changed:

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

--- Comment #14 from luoxhu at gcc dot gnu.org ---
Patch submmited:

https://gcc.gnu.org/pipermail/gcc-patches/2021-April/569255.html

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Xiong Hu Luo <luoxhu@gcc.gnu.org>:

https://gcc.gnu.org/g:9222481ffc69a6c0b73ec81e1bf04289fa3db0ed

commit r12-4757-g9222481ffc69a6c0b73ec81e1bf04289fa3db0ed
Author: Xionghu Luo <luoxhu@linux.ibm.com>
Date:   Wed Oct 27 21:21:20 2021 -0500

    rs6000: Fix wrong code generation for vec_sel [PR94613]

    The vsel instruction is a bit-wise select instruction.  Using an
    IF_THEN_ELSE to express it in RTL is wrong and leads to wrong code
    being generated in the combine pass.  Per element selection is a
    subset of per bit-wise selection,with the patch the pattern is
    written using bit operations.  But there are 8 different patterns
    to define "op0 := (op1 & ~op3) | (op2 & op3)":

    (~op3&op1) | (op3&op2),
    (~op3&op1) | (op2&op3),
    (op3&op2) | (~op3&op1),
    (op2&op3) | (~op3&op1),
    (op1&~op3) | (op3&op2),
    (op1&~op3) | (op2&op3),
    (op3&op2) | (op1&~op3),
    (op2&op3) | (op1&~op3),

    The latter 4 cases does not follow canonicalisation rules, non-canonical
    RTL is invalid RTL in vregs pass.  Secondly, combine pass will swap
    (op1&~op3) to (~op3&op1) by commutative canonical, which could reduce
    it to the FIRST 4 patterns, but it won't swap (op2&op3) | (~op3&op1) to
    (~op3&op1) | (op2&op3), so this patch handles it with 4 patterns with
    different NOT op3 position and check equality inside it.

    Tested pass on P7, P8 and P9.

    gcc/ChangeLog:

    2021-10-28  Xionghu Luo  <luoxhu@linux.ibm.com>

            PR target/94613
            * config/rs6000/altivec.md (*altivec_vsel<mode>): Change to ...
            (altivec_vsel<mode>): ... this and update define.
            (*altivec_vsel<mode>_uns): Delete.
            (altivec_vsel<mode>2): New define_insn.
            (altivec_vsel<mode>3): Likewise.
            (altivec_vsel<mode>4): Likewise.
            * config/rs6000/rs6000-call.c (altivec_expand_vec_sel_builtin):
New.
            (altivec_expand_builtin): Call altivec_expand_vec_sel_builtin to
expand
            vel_sel.
            * config/rs6000/rs6000.c (rs6000_emit_vector_cond_expr): Use
bit-wise
            selection instead of per element.
            * config/rs6000/vector.md:
            * config/rs6000/vsx.md (*vsx_xxsel<mode>): Change to ...
            (vsx_xxsel<mode>): ... this and update define.
            (*vsx_xxsel<mode>_uns): Delete.
            (vsx_xxsel<mode>2): New define_insn.
            (vsx_xxsel<mode>3): Likewise.
            (vsx_xxsel<mode>4): Likewise.

    gcc/testsuite/ChangeLog:

    2021-10-28  Xionghu Luo  <luoxhu@linux.ibm.com>

            PR target/94613
            * gcc.target/powerpc/pr94613.c: New test.

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[cvs-commit at gcc dot gnu.org]

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

--- Comment #16 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Xiong Hu Luo <luoxhu@gcc.gnu.org>:

https://gcc.gnu.org/g:5f9ef1339e9d0d709af6a70b60e584bf7decd761

commit r12-4758-g5f9ef1339e9d0d709af6a70b60e584bf7decd761
Author: Xionghu Luo <luoxhu@linux.ibm.com>
Date:   Wed Oct 27 21:22:39 2021 -0500

    rs6000: Fold xxsel to vsel since they have same semantics

    Fold xxsel to vsel like xxperm/vperm to avoid duplicate code.

    gcc/ChangeLog:

    2021-10-28  Xionghu Luo  <luoxhu@linux.ibm.com>

            PR target/94613
            * config/rs6000/altivec.md: Add vsx register constraints.
            * config/rs6000/vsx.md (vsx_xxsel<mode>): Delete.
            (vsx_xxsel<mode>2): Likewise.
            (vsx_xxsel<mode>3): Likewise.
            (vsx_xxsel<mode>4): Likewise.

    gcc/testsuite/ChangeLog:

    2021-10-28  Xionghu Luo  <luoxhu@linux.ibm.com>

            * gcc.target/powerpc/builtins-1.c: Adjust.

[Bug target/94613] S/390, powerpc: Wrong code generated for vec_sel builtin

[luoxhu at gcc dot gnu.org]

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

luoxhu at gcc dot gnu.org changed:

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

--- Comment #17 from luoxhu at gcc dot gnu.org ---
Fixed on master.