[Bug rtl-optimization/94665] New: missed minmax optimization opportunity for if/else structure.

[Bug rtl-optimization/94665] New: missed minmax optimization opportunity for if/else structure.

[z.zhanghaijian at huawei dot com]

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

            Bug ID: 94665
           Summary: missed minmax optimization opportunity for if/else
                    structure.
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: z.zhanghaijian at huawei dot com
  Target Milestone: ---

Minmax optimization for fortran，
for example:

SUBROUTINE mydepart(vara,varb,varc,res)
      REAL, INTENT(IN) :: vara,varb,varc
      REAL, INTENT(out) :: res

      res = vara
      if (res .lt. varb)  res = varb
      if (res .gt. varc)  res = varc
end SUBROUTINE

on aarch64, compile with -O2 -S -funsafe-math-optimizations, the asm:

ldr     s2, [x0]
ldr     s0, [x1]
ldr     s1, [x2]
fcmpe   s2, s0
fcsel   s0, s0, s2, mi
fminnm  s1, s1, s0
str     s1, [x3]
ret

The second if statement is optimized to fminnm, but the first can not.

In fact, it can be optimized to:

ldr     s2, [x0]
ldr     s1, [x1]
ldr     s0, [x2]
fmaxnm  s1, s2, s1
fminnm  s0, s0, s1
str     s0, [x3]

My proposal: I tracked the generation of fminnm is done in
simplify_if_then_else. The reason why the first statement optimization is not
done is that the conditions are not met:
rtx_equal_p (XEXP (cond, 0), true_rtx) && rtx_equal_p (XEXP (cond, 1),
false_rtx).

The RTX:

(if_then_else:SF (lt (reg:SF 92 [ _1 ])
        (reg:SF 93 [ _2 ]))
    (reg:SF 93 [ _2 ])
    (reg:SF 92 [ _1 ]))

We can swap the true_rtx/false_rtx, and take the maximum.

the patch:

diff --git a/gcc/combine.c b/gcc/combine.c
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -6641,25 +6641,43 @@ simplify_if_then_else (rtx x)

   if ((! FLOAT_MODE_P (mode) || flag_unsafe_math_optimizations)
       && comparison_p
-      && rtx_equal_p (XEXP (cond, 0), true_rtx)
-      && rtx_equal_p (XEXP (cond, 1), false_rtx)
       && ! side_effects_p (cond))
-    switch (true_code)
-      {
-      case GE:
-      case GT:
-       return simplify_gen_binary (SMAX, mode, true_rtx, false_rtx);
-      case LE:
-      case LT:
-       return simplify_gen_binary (SMIN, mode, true_rtx, false_rtx);
-      case GEU:
-      case GTU:
-       return simplify_gen_binary (UMAX, mode, true_rtx, false_rtx);
-      case LEU:
-      case LTU:
-       return simplify_gen_binary (UMIN, mode, true_rtx, false_rtx);
-      default:
-       break;
+    {
+      int swapped = 0;
+      if (rtx_equal_p (XEXP (cond, 0), false_rtx)
+         && rtx_equal_p (XEXP (cond, 1), true_rtx))
+       {
+         std::swap (true_rtx, false_rtx);
+         swapped = 1;
+       }
+
+      if (rtx_equal_p (XEXP (cond, 0), true_rtx)
+         && rtx_equal_p (XEXP (cond, 1), false_rtx))
+       switch (true_code)
+         {
+         case GE:
+         case GT:
+           return simplify_gen_binary (swapped ? SMIN : SMAX,
+                                       mode, true_rtx, false_rtx);
+         case LE:
+         case LT:
+           return simplify_gen_binary (swapped ? SMAX : SMIN,
+                                       mode, true_rtx, false_rtx);
+         case GEU:
+         case GTU:
+           return simplify_gen_binary (swapped ? UMIN : UMAX,
+                                       mode, true_rtx, false_rtx);
+         case LEU:
+         case LTU:
+           return simplify_gen_binary (swapped ? UMAX : UMIN,
+                                       mode, true_rtx, false_rtx);
+         default:
+           break;
+         }
+
+      /* Restore if not MIN or MAX.  */
+      if (swapped)
+       std::swap (true_rtx, false_rtx);
       }

   /* If we have (if_then_else COND (OP Z C1) Z) and OP is an identity when its

Any suggestions?

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This should have been done at gimple level in PHIOPT (when using -ffast-math). 
I wonder why it was not.

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[segher at gcc dot gnu.org]

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

--- Comment #2 from Segher Boessenkool <segher at gcc dot gnu.org> ---
If vara is a NaN, this is not the same; it needs -ffinite-math-only.
And in fact adding that option does the trick (on powerpc that is, I
don't have an aarch64 Fortran handy).

Could you check this please?

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[segher at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-04-20
     Ever confirmed|0                           |1
                 CC|                            |segher at gcc dot gnu.org
             Status|UNCONFIRMED                 |WAITING

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|WAITING                     |RESOLVED

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Segher Boessenkool from comment #2)
> If vara is a NaN, this is not the same; it needs -ffinite-math-only.
> And in fact adding that option does the trick (on powerpc that is, I
> don't have an aarch64 Fortran handy).
> 
> Could you check this please?

I was testing and it is done with -ffinite-math-only as it done on the gimple
level as I mentioned in my comment #1 just before you did your comment.

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[z.zhanghaijian at huawei dot com]

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

--- Comment #4 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
(In reply to Segher Boessenkool from comment #2)
> If vara is a NaN, this is not the same; it needs -ffinite-math-only.
> And in fact adding that option does the trick (on powerpc that is, I
> don't have an aarch64 Fortran handy).
> 
> Could you check this please?

Yes, on aarch64, fmaxnm can be generated with -ffinite-math-only and
-funsafe-math-optimizations.

One question: why is it OK for rtl combine to generate the fminnm here?
Anything I missed?

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[segher at gcc dot gnu.org]

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

--- Comment #5 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Can you show the  -fdump-rtl-combine-all  dump where that insn is
created?

It is fine to generate min or max insns here; but you need to handle the 
case where vara is NaN: you should return that NaN then.  Other than that
your function is just the max of vara, varb, varc.

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[z.zhanghaijian at huawei dot com]

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

--- Comment #6 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
(In reply to Segher Boessenkool from comment #5)
> Can you show the  -fdump-rtl-combine-all  dump where that insn is
> created?
> 
> It is fine to generate min or max insns here; but you need to handle the 
> case where vara is NaN: you should return that NaN then.  Other than that
> your function is just the max of vara, varb, varc.

The dump info：

Trying 39 -> 40:
   39: cc:CCFPE=cmp(r94:SF,r93:SF)
   40: r94:SF={(cc:CCFPE<0)?r94:SF:r93:SF}
      REG_DEAD r93:SF
      REG_DEAD cc:CCFPE
Successfully matched this instruction:
(set (reg:SF 94 [ _4 ])
    (smin:SF (reg:SF 94 [ _4 ])
        (reg:SF 93 [ _2 ])))
allowing combination of insns 39 and 40
original costs 4 + 4 = 8
replacement cost 8
deferring deletion of insn with uid = 39.
modifying insn i3    40: r94:SF=smin(r94:SF,r93:SF)
      REG_DEAD r93:SF
deferring rescan insn with uid = 40.

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[segher at gcc dot gnu.org]

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

--- Comment #7 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Can r94 or r93 be NaN there?

(I should build an aarch64 compiler...  takes almost a day though :-) )

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[z.zhanghaijian at huawei dot com]

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

--- Comment #8 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
(In reply to Segher Boessenkool from comment #7)
> Can r94 or r93 be NaN there?
> 
> (I should build an aarch64 compiler...  takes almost a day though :-) )

Yes, r94 and r93 are function arguments, there is no limit in the example, it
may be NaN. 

-funsafe-math-optimizations allow optimization assume that arguments are not
NaNs like -ffinite-math-only？

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[segher at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---

--- Comment #9 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Okay, native aarch64 compiler built.  Phew.

vara, varb, varc are in regs r92, r93, r94.  The code before combine is:

37: cc:CCFPE=cmp(r92:SF,r93:SF)
38: r93:SF={(cc:CCFPE<0)?r93:SF:r92:SF}
      REG_DEAD r92:SF
      REG_DEAD cc:CCFPE
39: cc:CCFPE=cmp(r94:SF,r93:SF)
40: r94:SF={(cc:CCFPE<0)?r94:SF:r93:SF}
      REG_DEAD r93:SF
      REG_DEAD cc:CCFPE

and it leaves 37+38 as it was:
Trying 37 -> 38:
   37: cc:CCFPE=cmp(r92:SF,r93:SF)
   38: r93:SF={(cc:CCFPE<0)?r93:SF:r92:SF}
      REG_DEAD r92:SF
      REG_DEAD cc:CCFPE
Failed to match this instruction:
(set (reg:SF 93 [ _2 ])
    (if_then_else:SF (lt (reg:SF 92 [ _1 ])
            (reg:SF 93 [ _2 ]))
        (reg:SF 93 [ _2 ])
        (reg:SF 92 [ _1 ])))

but it combines 39+40:
Trying 39 -> 40:
   39: cc:CCFPE=cmp(r94:SF,r93:SF)
   40: r94:SF={(cc:CCFPE<0)?r94:SF:r93:SF}
      REG_DEAD r93:SF
      REG_DEAD cc:CCFPE
Successfully matched this instruction:
(set (reg:SF 94 [ _4 ])
    (smin:SF (reg:SF 94 [ _4 ])
        (reg:SF 93 [ _2 ])))
allowing combination of insns 39 and 40
original costs 4 + 4 = 8
replacement cost 8
deferring deletion of insn with uid = 39.
modifying insn i3    40: r94:SF=smin(r94:SF,r93:SF)
      REG_DEAD r93:SF
deferring rescan insn with uid = 40.

So huh, simplify_if_then_else seems to be buggy:
  /* Look for MIN or MAX.  */

  if ((! FLOAT_MODE_P (mode) || flag_unsafe_math_optimizations)
      && comparison_p
      && rtx_equal_p (XEXP (cond, 0), true_rtx)
      && rtx_equal_p (XEXP (cond, 1), false_rtx)
      && ! side_effects_p (cond))

that isn't correct afaics?

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[segher at gcc dot gnu.org]

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

--- Comment #10 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(Because it should handle NaNs, and SMAX etc. do not).

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[segher at gcc dot gnu.org]

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

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

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

--- Comment #11 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Confirmed the comment 4 problem, on all archs.  This is a very old bug.

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[z.zhanghaijian at huawei dot com]

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

--- Comment #12 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
(In reply to Segher Boessenkool from comment #11)
> Confirmed the comment 4 problem, on all archs.  This is a very old bug.

Ok to me, can this optimization change flag_unsafe_math_optimizations to
flag_finite_math_only?

Like the patch:

diff --git a/gcc/combine.c b/gcc/combine.c
index cff76cd3303..f394d8dfd03 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -6643,7 +6643,7 @@ simplify_if_then_else (rtx x)

   /* Look for MIN or MAX.  */

-  if ((! FLOAT_MODE_P (mode) || flag_unsafe_math_optimizations)
+  if ((! FLOAT_MODE_P (mode) || flag_finite_math_only)
       && comparison_p
       && rtx_equal_p (XEXP (cond, 0), true_rtx)
       && rtx_equal_p (XEXP (cond, 1), false_rtx)

Can this fix the bug？

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[z.zhanghaijian at huawei dot com]

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

--- Comment #13 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
When change to flag_finite_math_only, this fmaxnm can also be generated with
the patch above(swap the true_rtx/false_rtx).

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[z.zhanghaijian at huawei dot com]

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

--- Comment #14 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
(In reply to Segher Boessenkool from comment #11)
> Confirmed the comment 4 problem, on all archs.  This is a very old bug.

There are two ways to fix this bug:
1. Change flag_unsafe_math_optimizations to flag_finite_math_only, so that
fmaxnm/fminnm can be generated under -ffinite-math-only;
2. Delete this optimization.
Which one do you prefer?

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[segher at gcc dot gnu.org]

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

--- Comment #15 from Segher Boessenkool <segher at gcc dot gnu.org> ---
replacing flag_unsafe_math_operations by flag_finite_math_only isn't correct,
but you can add it instead, i.e.

-  if ((! FLOAT_MODE_P (mode) || flag_unsafe_math_optimizations)
+  if (!FLOAT_MODE_P (mode)
+      || (flag_unsafe_math_optimizations && flag_finite_math_only))

or such?

Thanks for working on a patch!

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[z.zhanghaijian at huawei dot com]

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

--- Comment #16 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
(In reply to Segher Boessenkool from comment #15)
> replacing flag_unsafe_math_operations by flag_finite_math_only isn't correct,
> but you can add it instead, i.e.
> 
> -  if ((! FLOAT_MODE_P (mode) || flag_unsafe_math_optimizations)
> +  if (!FLOAT_MODE_P (mode)
> +      || (flag_unsafe_math_optimizations && flag_finite_math_only))
> 
> or such?
> 
> Thanks for working on a patch!


Ok, I will create a new PR to track this bug, and I will submit a bugfix patch
whit that PR.

In addition, I tracked the process of generating fmaxnm/fminnm and found that
it was generated in phiopt (minmax_replacement) and if-conversion
(noce_try_minmax). In the rtl combine, only fminnm can be generated. Is it
necessary for us to improve this optimization in the rtl combine using the
above patch in stage1?

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[segher at gcc dot gnu.org]

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

--- Comment #17 from Segher Boessenkool <segher at gcc dot gnu.org> ---
[ Please don't add other email addresses for me; I get enough mail already,
  I don't need all bugzilla mail in duplicate :-) ]

(In reply to z.zhanghaijian@huawei.com from comment #16)
> Ok, I will create a new PR to track this bug, and I will submit a bugfix
> patch whit that PR.

You can make this PR RESOLVED again, after you made a new PR.

> In addition, I tracked the process of generating fmaxnm/fminnm and found
> that it was generated in phiopt (minmax_replacement) and if-conversion
> (noce_try_minmax). In the rtl combine, only fminnm can be generated. Is it
> necessary for us to improve this optimization in the rtl combine using the
> above patch in stage1?

Yeah, ifcvt will often do it.

combine can handle max just fine as well; you'll need to track down why
it doesn't here (I noticed it doesn't as well, it wasn't immediately
obvious to me what the difference with the min case is).

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[z.zhanghaijian at huawei dot com]

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

--- Comment #18 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
(In reply to Segher Boessenkool from comment #17)
> [ Please don't add other email addresses for me; I get enough mail already,
>   I don't need all bugzilla mail in duplicate :-) ]


OK

> (In reply to z.zhanghaijian@huawei.com from comment #16)
> > Ok, I will create a new PR to track this bug, and I will submit a bugfix
> > patch whit that PR.
> 
> You can make this PR RESOLVED again, after you made a new PR.


OK, the new PR is PR94708, I will make this PR RESOLVED.

> 
> > In addition, I tracked the process of generating fmaxnm/fminnm and found
> > that it was generated in phiopt (minmax_replacement) and if-conversion
> > (noce_try_minmax). In the rtl combine, only fminnm can be generated. Is it
> > necessary for us to improve this optimization in the rtl combine using the
> > above patch in stage1?
> 
> Yeah, ifcvt will often do it.
> 
> combine can handle max just fine as well; you'll need to track down why
> it doesn't here (I noticed it doesn't as well, it wasn't immediately
> obvious to me what the difference with the min case is).


I will continue to track why fmaxnm is not generated.

[Bug rtl-optimization/94665] missed minmax optimization opportunity for if/else structure.

[z.zhanghaijian at huawei dot com]

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

z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> changed:

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

--- Comment #19 from z.zhanghaijian at huawei dot com <z.zhanghaijian at huawei dot com> ---
Resolved.