[PATCH v2] rs6000: Support UN[GL][ET] in rs6000_maybe_emit_maxc_minc [PR105002]

[PATCH v2] rs6000: Support UN[GL][ET] in rs6000_maybe_emit_maxc_minc [PR105002]

[Kewen.Lin]

Hi,

Commit r12-7687 exposed one miss optimization chance in function
rs6000_maybe_emit_maxc_minc, for now it only considers comparison
codes GE/GT/LE/LT, but it can support more variants with codes
UNLT/UNLE/UNGT/UNGE by reversing them into the equivalent ones
with GE/GT/LE/LT.

Bootstrapped and regtested on powerpc64-linux-gnu P8 and
powerpc64le-linux-gnu P9 and P10.

Is it ok for trunk?

v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/592204.html

v2: Add more comments to provide better readability as Segher suggested.

BR,
Kewen
-----
	PR target/105002

gcc/ChangeLog:

	* config/rs6000/rs6000.cc (rs6000_maybe_emit_maxc_minc): Support more
	comparison codes UNLT/UNLE/UNGT/UNGE.

---
 gcc/config/rs6000/rs6000.cc | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index f41b8f740ba..40db13e6498 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -15838,11 +15838,26 @@ rs6000_maybe_emit_maxc_minc (rtx dest, rtx op, rtx true_cond, rtx false_cond)
   rtx op1 = XEXP (op, 1);
   machine_mode compare_mode = GET_MODE (op0);
   machine_mode result_mode = GET_MODE (dest);
-  bool max_p = false;
+  bool max_p;

   if (result_mode != compare_mode)
     return false;

+  /* See the comments of this function, it simply expects GE/GT/LE/LT in
+     the checks, but for the reversible equivalent UNLT/UNLE/UNGT/UNGE,
+     we need to do the reversions first to make the following checks
+     support fewer cases, like:
+
+	(a UNLT b) ? op1 : op2 =>  (a >= b) ? op2 : op1;
+	(a UNLE b) ? op1 : op2 =>  (a >  b) ? op2 : op1;
+	(a UNGT b) ? op1 : op2 =>  (a <= b) ? op2 : op1;
+	(a UNGE b) ? op1 : op2 =>  (a <  b) ? op2 : op1;  */
+  if (code == UNGE || code == UNGT || code == UNLE || code == UNLT)
+    {
+      code = reverse_condition_maybe_unordered (code);
+      std::swap (true_cond, false_cond);
+    }
+
   if (code == GE || code == GT)
     max_p = true;
   else if (code == LE || code == LT)
--
2.27.0

Re: [PATCH v2] rs6000: Support UN[GL][ET] in rs6000_maybe_emit_maxc_minc [PR105002]

[Segher Boessenkool]

Hi!

On Fri, Apr 01, 2022 at 02:27:14PM +0800, Kewen.Lin wrote:
> Commit r12-7687 exposed one miss optimization chance in function
> rs6000_maybe_emit_maxc_minc, for now it only considers comparison
> codes GE/GT/LE/LT, but it can support more variants with codes
> UNLT/UNLE/UNGT/UNGE by reversing them into the equivalent ones
> with GE/GT/LE/LT.

You may want to add somewhere (in the comment in the code perhaps?)
that if we see e.g. UNLT it guarantees that we have 4-way condition
codes (LT/GT/EQ/UN), so we do not have to check for fast-math or the
like.  This is always true of course, but it doesn't hurt to remind
the reader :-)

The PR marker goes here:

	PR target/105002
> 	* config/rs6000/rs6000.cc (rs6000_maybe_emit_maxc_minc): Support more
> 	comparison codes UNLT/UNLE/UNGT/UNGE.

> -  bool max_p = false;
> +  bool max_p;

Please move this to later, since you touch it anyway:

  bool max_p;
>    if (code == GE || code == GT)
>      max_p = true;
>    else if (code == LE || code == LT)

Okay for trunk with those finishing touches.  Thanks!


Segher

Re: [PATCH v2] rs6000: Support UN[GL][ET] in rs6000_maybe_emit_maxc_minc [PR105002]

[Kewen.Lin]

on 2022/4/1 10:49 PM, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Apr 01, 2022 at 02:27:14PM +0800, Kewen.Lin wrote:
>> Commit r12-7687 exposed one miss optimization chance in function
>> rs6000_maybe_emit_maxc_minc, for now it only considers comparison
>> codes GE/GT/LE/LT, but it can support more variants with codes
>> UNLT/UNLE/UNGT/UNGE by reversing them into the equivalent ones
>> with GE/GT/LE/LT.
> 
> You may want to add somewhere (in the comment in the code perhaps?)
> that if we see e.g. UNLT it guarantees that we have 4-way condition
> codes (LT/GT/EQ/UN), so we do not have to check for fast-math or the
> like.  This is always true of course, but it doesn't hurt to remind
> the reader :-)
> 
> The PR marker goes here:
> 
> 	PR target/105002
>> 	* config/rs6000/rs6000.cc (rs6000_maybe_emit_maxc_minc): Support more
>> 	comparison codes UNLT/UNLE/UNGT/UNGE.
> 

The current daily bump seems smart and as verified with gcc-verify that
it's able to add the PR marker into the (each if there are multiple) place
that needs it.

But anyway I updated the commit log as you suggested.  :)

>> -  bool max_p = false;
>> +  bool max_p;
> 
> Please move this to later, since you touch it anyway:
> 
>   bool max_p;
>>    if (code == GE || code == GT)
>>      max_p = true;
>>    else if (code == LE || code == LT)
> 
> Okay for trunk with those finishing touches.  Thanks!
> 

Thanks for the review!  Addressed the comments and committed as r12-8010.


BR,
Kewen