[Bug tree-optimization/104645] New: [12 Regression] i ? i % 2 : 0 not optimized anymore

[Bug tree-optimization/104645] New: [12 Regression] i ? i % 2 : 0 not optimized anymore

[denis.campredon at gmail dot com]

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

            Bug ID: 104645
           Summary: [12 Regression] i ? i % 2 : 0 not optimized anymore
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: denis.campredon at gmail dot com
  Target Milestone: ---

Was told to file a new PR for that case :

------------------
int foo(unsigned i) {
    return i ? i % 2 : 0;
}
------------------

With trunk
------------------------
foo(unsigned int):
        mov     eax, edi
        xor     edx, edx
        and     eax, 1
        test    edi, edi
        cmove   eax, edx
        ret
-----------------------

With 11.2
-----------------------
foo(unsigned int):
        mov     eax, edi
        and     eax, 1
        ret
-----------------------

According to Jakub Jelinek in PR104639 it started with
r12-5358-g32221357007666124409ec3ee0d3a1cf263ebc9e

[Bug tree-optimization/104645] [12 Regression] i ? i % 2 : 0 not optimized anymore

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0
           Keywords|                            |missed-optimization
                 CC|                            |pinskia at gcc dot gnu.org

[Bug tree-optimization/104645] [12 Regression] i ? i % 2 : 0 not optimized anymore

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
     Ever confirmed|0                           |1
                 CC|                            |jakub at gcc dot gnu.org
   Last reconfirmed|                            |2022-02-22
             Status|UNCONFIRMED                 |NEW

[Bug tree-optimization/104645] [12 Regression] i ? i % 2 : 0 not optimized anymore

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |pinskia at gcc dot gnu.org

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I don't think this should be a P1.
Here is another testcase which shows the same issue but is not a regression:
int foo(unsigned i) {
    int t = 0;
    if (i)
    {
       unsigned tt = i % 2u;
       t = tt;
    }
    return t;
}
int foo1(unsigned i) {
    unsigned t = i % 2;
    unsigned tt =  i ? t : 0;
    return tt;
}
---- CUT ----
For some reason the code in value_replacement is not able to handle the
different IR.
I have some ideas on how to fix this for GCC 13 though.

[Bug tree-optimization/104645] [12 Regression] i ? i % 2 : 0 not optimized anymore

[pinskia at gcc dot gnu.org]

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

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

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

[Bug tree-optimization/104645] [12 Regression] i ? i % 2 : 0 not optimized anymore

[rguenth at gcc dot gnu.org]

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

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
So it's unhandled

  <bb 2> :
  if (i_3(D) != 0)
    goto <bb 3>; [INV]
  else
    goto <bb 4>; [INV]

  <bb 3> :
  _4 = i_3(D) & 1;
  iftmp.0_5 = (int) _4;

  <bb 4> :
  # iftmp.0_2 = PHI <iftmp.0_5(3), 0(2)>
  return iftmp.0_2;

it also doesn't handle

  <bb 2> :
  if (i_3(D) != 0)
    goto <bb 3>; [INV]
  else
    goto <bb 4>; [INV]

  <bb 3> :
  i.1_1 = (int) i_3(D);
  iftmp.0_5 = i.1_1 & 1;

  <bb 4> :
  # iftmp.0_2 = PHI <iftmp.0_5(3), 0(2)>
  return iftmp.0_2;

I wonder if we can't cook up a fix for GCC 12 though since it _is_ a regression
after all.  Just look through (nop-)conversions somehow.

[Bug tree-optimization/104645] [12 Regression] i ? i % 2 : 0 not optimized anymore

[jakub at gcc dot gnu.org]

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

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

I wonder if at least for GCC 13 we just can't treat even that cast stmt as a
preparation statement.
This won't handle say shifts etc. with casts after them, but will at least fix
this regression.
And for GCC 13 perhaps we can throw away all the separation of "preparation"
and assign statements and natural_element_p and absorbing_element_p and instead
just try to constant evaluate all the middle_bb statements and see if that
doesn't trigger traps/overflows/other UB and yields the expected PHI arg
constant from the original comparison constant.

[Bug tree-optimization/104645] [12 Regression] i ? i % 2 : 0 not optimized anymore

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The first GCC 13 should have been GCC 12 of course.

[Bug tree-optimization/104645] [12 Regression] i ? i % 2 : 0 not optimized anymore

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 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:d9c03fc27d8147a9401a29739694b214df48a9a2

commit r12-7952-gd9c03fc27d8147a9401a29739694b214df48a9a2
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Apr 1 11:50:41 2022 +0200

    phiopt: Improve value_replacement [PR104645]

    The following patch fixes the P1 regression by reusing existing
    value_replacement code.  That function already has code to
    handle simple preparation statements (casts, and +,&,|,^ binary
    assignments) before a final binary assignment (which can be
    much wider range of ops).  When we have e.g.
          if (y_3(D) == 0)
            goto <bb 4>;
          else
            goto <bb 3>;
         <bb 3>:
          y_4 = y_3(D) & 31;
          _1 = (int) y_4;
          _6 = x_5(D) r<< _1;
         <bb 4>:
          # _2 = PHI <x_5(D)(2), _6(3)>
    the preparation statements y_4 = y_3(D) & 31; and
    _1 = (int) y_4; are handled by constant evaluation, passing through
    y_3(D) = 0 initially and propagating that through the assignments
    with checking that UB isn't invoked.  But the final
    _6 = x_5(D) r<< _1; assign is handled differently, either through
    neutral_element_p or absorbing_element_p.
    In the first function below we now have:
      <bb 2> [local count: 1073741824]:
      if (i_2(D) != 0)
        goto <bb 3>; [50.00%]
      else
        goto <bb 4>; [50.00%]

      <bb 3> [local count: 536870913]:
      _3 = i_2(D) & 1;
      iftmp.0_4 = (int) _3;

      <bb 4> [local count: 1073741824]:
      # iftmp.0_1 = PHI <iftmp.0_4(3), 0(2)>
    where in GCC 11 we had:
      <bb 2> :
      if (i_3(D) != 0)
        goto <bb 3>; [INV]
      else
        goto <bb 4>; [INV]

      <bb 3> :
      i.1_1 = (int) i_3(D);
      iftmp.0_5 = i.1_1 & 1;

      <bb 4> :
      # iftmp.0_2 = PHI <iftmp.0_5(3), 0(2)>
    Current value_replacement can handle the latter as the last
    stmt of middle_bb is a binary op that in this case satisfies
    absorbing_element_p.
    But the former we can't handle, as the last stmt in middle_bb
    is a cast.

    The patch makes it work in that case by pretending all of middle_bb
    are the preparation statements and there is no binary assign at the
    end, so everything is handled through the constant evaluation.
    We simply set at the start of middle_bb the lhs of comparison
    virtually to the rhs, propagate it through and at the end
    see if virtually the arg0 of the PHI is equal to arg1 of it.

    For GCC 13, I think we just should throw away all the neutral/absorbing
    element stuff and do the constant evaluation of the whole middle_bb
    and handle that way all the ops we currently handle in neutral/absorbing
    element.

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

            PR tree-optimization/104645
            * tree-ssa-phiopt.cc (value_replacement): If assign has
            CONVERT_EXPR_CODE_P rhs_code, treat it like a preparation
            statement with constant evaluation.

            * gcc.dg/tree-ssa/pr104645.c: New test.

[Bug tree-optimization/104645] [12 Regression] i ? i % 2 : 0 not optimized anymore

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.