[Bug tree-optimization/105314] New: ifcvt regression in noce_try_store_flag_mask

[Bug tree-optimization/105314] New: ifcvt regression in noce_try_store_flag_mask

[christophm30 at gmail dot com]

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

            Bug ID: 105314
           Summary: ifcvt regression in noce_try_store_flag_mask
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: christophm30 at gmail dot com
  Target Milestone: ---

The function noce_try_store_flag_mask() in ifcvt.cc converts
"if (test) x = 0;" to "x &= -(test == 0);" (if costs permit that).

Commit 3a7ba8fd (which was introduced a month ago to fix PR104960) triggers a
regression so that the if-conversion can't be performed anymore.

On RISC-V this manifests as follows:
"""
long func(long a, long b, long c)
{
  if (c)
    a = 0;
  return a;
}
"""

Old code:
0000000000000000 <func>:
   0:   00163613                seqz    a2,a2
   4:   40c00633                neg     a2,a2
   8:   8d71                    and     a0,a0,a2
   a:   8082                    ret

New (branching) code:
0000000000000000 <func>:
   0:   c211                    beqz    a2,4 <.L1>
   2:   4501                    li      a0,0
0000000000000004 <.L1>:
   4:   8082                    ret

Looking through the test suite, I could only find the file
"gcc.target/arm/ifcvt-size-check.c" which should be affected by this regression
as well. However, I haven't tested that.

[Bug tree-optimization/105314] ifcvt regression in noce_try_store_flag_mask

[christophm30 at gmail dot com]

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

--- Comment #1 from Christoph Müllner <christophm30 at gmail dot com> ---
A first analysis in noce_try_store_flag_mask() showed the following observation
in case of the failing conversion into the branchless sequence:

* if_info->b == const0_rtx holds
* rtx_equal_p (if_info->a, if_info->x) does not hold

Assuming that the generic form would be "x := c ? x : 0", the second
observation seems odd.

[Bug tree-optimization/105314] ifcvt regression in noce_try_store_flag_mask

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

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

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
> Commit 3a7ba8fd (which was introduced a month ago to fix PR104960) triggers

g:3a7ba8fd

[Bug tree-optimization/105314] ifcvt regression in noce_try_store_flag_mask

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-04-20
                 CC|                            |rguenth at gcc dot gnu.org

[Bug target/105314] ifcvt regression in noce_try_store_flag_mask

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|tree-optimization           |target
           Keywords|                            |missed-optimization
             Target|                            |riscv

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
I observed sth similar on arm and x86_64 where the backends were adjusted to
treat two equal variants the same.  The issue that is observed from the
GIMPLE level change is that the edge containing the forwarder changes and
so we get slightly different initial RTL out of it.

You have to analyze in more detail what happens.

[Bug target/105314] [12 Regression] ifcvt regression in noce_try_store_flag_mask

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|ifcvt regression in         |[12 Regression] ifcvt
                   |noce_try_store_flag_mask    |regression in
                   |                            |noce_try_store_flag_mask
   Target Milestone|---                         |12.0

[Bug rtl-optimization/105314] [12 Regression] ifcvt regression in noce_try_store_flag_mask

[jakub at gcc dot gnu.org]

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

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

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

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

Untested fix.

[Bug rtl-optimization/105314] [12 Regression] ifcvt regression in noce_try_store_flag_mask

[ptomsich at gcc dot gnu.org]

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

--- Comment #5 from ptomsich at gcc dot gnu.org ---
The fix addresses the issue and generates no new failures on small test cases.
Testing against SPEC is still ongoing and I'll report back once that has
completed.

[Bug rtl-optimization/105314] [12 Regression] ifcvt regression in noce_try_store_flag_mask

[christophm30 at gmail dot com]

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

--- Comment #6 from Christoph Müllner <christophm30 at gmail dot com> ---
The proposed fix is triggering an invalid transformation.

The pattern we need to transform is:
  Convert "if (test) x = 0;" to "x &= -(test == 0);"

If there is an else branch, we can't apply the transformation.

Error case:
"""
long func2 (long a, long b, long c)  // should not be optimized
{
    if (c)
        a = 0;
    else
        a = 5;
    return a;
}
"""

With the patch this gets compiled to:
0000000000000006 <func2>:
   6:   00163513                seqz    a0,a2
   a:   40a00533                neg     a0,a0
   e:   8915                    andi    a0,a0,5
  10:   8082                    ret

[Bug rtl-optimization/105314] [12 Regression] ifcvt regression in noce_try_store_flag_mask

[ptomsich at gcc dot gnu.org]

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

--- Comment #7 from ptomsich at gcc dot gnu.org ---
The transformation for 
"""
long func2 (long a, long b, long c)
{
    if (c)
        a = 0;
    else
        a = 5;
    return a;
}
"""
into
"""
0000000000000006 <func2>:
   6:   00163513                seqz    a0,a2
   a:   40a00533                neg     a0,a0
   e:   8915                    andi    a0,a0,5
  10:   8082                    ret
"""
is correct, as we get
"""
  tmp = c ? 0 : -1;
  a = tmp & 5;
"""

[Bug rtl-optimization/105314] [12 Regression] ifcvt regression in noce_try_store_flag_mask

[christophm30 at gmail dot com]

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

--- Comment #8 from Christoph Müllner <christophm30 at gmail dot com> ---
Yes, I was wrong in my previous comment.
Jakub's patch is of course right.

The transformation in noce_try_store_flag_mask() does:

  x = cond ? 0 else b  // b may be x

==>

  target = cond ? 0 : -1;  // seqz & neg
  x = target & b;          // and

I was misled by the comment above the function,
which only shows the special case when b equals x.

Probably this is a better comment:

/* Convert "if (test) x = 0 else x = y;" to "x = -(test == 0) & y;"  */

[Bug rtl-optimization/105314] [12 Regression] ifcvt regression in noce_try_store_flag_mask

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:7d31c678d68d7b6820a958584619ca763b0eb9c5

commit r12-8264-g7d31c678d68d7b6820a958584619ca763b0eb9c5
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Apr 26 10:11:58 2022 +0200

    ifcvt: Improve noce_try_store_flag_mask [PR105314]

    The following testcase regressed on riscv due to the splitting of critical
    edges in the sink pass, similarly to x86_64 compared to GCC 11 we now swap
    the edges, whether true or false edge goes to an empty forwarded bb.
    From GIMPLE POV, those 2 forms are equivalent, but as can be seen here, for
    some ifcvt opts it matters one way or another.

    On this testcase, noce_try_store_flag_mask used to trigger and transformed
    if (pseudo2) pseudo1 = 0;
    into
    pseudo1 &= -(pseudo2 == 0);
    But with the swapped edges ifcvt actually sees
    if (!pseudo2) pseudo3 = pseudo1; else pseudo3 = 0;
    and noce_try_store_flag_mask punts.  IMHO there is no reason why it
    should punt those, it is equivalent to
    pseudo3 = pseudo1 & -(pseudo2 == 0);
    and especially if the target has 3 operand AND, it shouldn't be any more
    costly (and even with 2 operand AND, it might very well happen that RA
    can make it happen without any extra moves).

    Initially I've just removed the rtx_equal_p calls from the conditions
    and didn't add anything there, but that broke aarch64 bootstrap and
    regressed some testcases on x86_64, where if_info->a or if_info->b could be
    some larger expression that we can't force into a register.
    Furthermore, the case where both if_info->a and if_info->b are constants is
    better handled by other ifcvt optimizations like noce_try_store_flag
    or noce_try_inverse_constants or noce_try_store_flag_constants.
    So, I've restricted it to just a REG (perhaps SUBREG of REG might be ok
too)
    next to what has been handled previously.

    2022-04-26  Jakub Jelinek  <jakub@redhat.com>

            PR rtl-optimization/105314
            * ifcvt.cc (noce_try_store_flag_mask): Don't require that the
non-zero
            operand is equal to if_info->x, instead use the non-zero operand
            as one of the operands of AND with if_info->x as target.

            * gcc.target/riscv/pr105314.c: New test.

[Bug rtl-optimization/105314] [12 Regression] ifcvt regression in noce_try_store_flag_mask

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Hopefully fixed.
The 2 constant cases are handled in other noce_try* routines.

[Bug rtl-optimization/105314] [12 Regression] ifcvt regression in noce_try_store_flag_mask

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Maciej W. Rozycki <macro@gcc.gnu.org>:

https://gcc.gnu.org/g:3e3b9b708d390074f7825401b808e0ed41552c1d

commit r14-8459-g3e3b9b708d390074f7825401b808e0ed41552c1d
Author: Maciej W. Rozycki <macro@embecosm.com>
Date:   Fri Jan 26 21:47:40 2024 +0000

    RISC-V/testsuite: Also verify if-conversion runs for pr105314.c

    Verify that if-conversion succeeded through noce_try_store_flag_mask, as
    per PR rtl-optimization/105314, tightening the test case and making it
    explicit.

            gcc/testsuite/
            * gcc.target/riscv/pr105314.c: Scan the RTL "ce1" pass too.