Message ID | bdb4418f-d186-793f-5cc4-a7fbc5c1550c@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [v2] rs6000: Support UN[GL][ET] in rs6000_maybe_emit_maxc_minc [PR105002] | expand |
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
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
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)