Don't do int cmoves for IEEE comparisons, PR target/104256.
Commit Message
Don't do int cmoves for IEEE comparisons, PR target/104256.
Protect int cmove from raising an assertion if it is trying to do an int
conditional move where the test involves floating point comparisons that
can't easily be reversed due to NaNs.
The code used to generate the condition, and possibly reverse the condition if
ISEL could not handle it by rewriting the OP in the comparison rtx.
Unfortunately there are some conditions like UNLE that can't easily be reversed
due to NaNs.
The patch changes the code so that it does the reversal before generating the
comparison. If the comparison cannot be reversed, it just returns false,
which indicates that we can't do an int conditional move in this case.
I have tested this on a little endian power9 system doing a bootstrap. There
were no regressions. Can I install this in the trunk? Neither GCC 10 nor GCC
11 seem to generate an assertion faiure, so I don't plan to backport it.
2022-02-17 Michael Meissner <meissner@the-meissners.org>
gcc/
PR target/104256
* config/rs6000/rs6000.cc (rs6000_emit_int_cmove): Don't do
integer conditional moves if the test needs to be reversed and
there isn't a direct reverse comparison.
gcc/testsuite/
PR target/104256
* gcc.target/powerpc/ppc-fortran/pr104254.f90: New test.
---
gcc/config/rs6000/rs6000.cc | 36 ++++++++++---------
.../powerpc/ppc-fortran/pr104254.f90 | 25 +++++++++++++
2 files changed, 44 insertions(+), 17 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/ppc-fortran/pr104254.f90
Comments
Hi!
First, you need to adjust after Robin's patch, and retest.
On Thu, Feb 17, 2022 at 01:56:04PM -0500, Michael Meissner wrote:
> Don't do int cmoves for IEEE comparisons, PR target/104256.
> Unfortunately there are some conditions like UNLE that can't easily be reversed
> due to NaNs.
What do you mean? The reverse of UNLE is GT. You don't even need to
check if fast-math is active, or whether this is FP at all -- that is a
*given* if you see UNLE!
You need more context to reverse GT. For fast-math and integer that
gives LE, for fp it is UNLE.
> The patch changes the code so that it does the reversal before generating the
> comparison. If the comparison cannot be reversed, it just returns false,
> which indicates that we can't do an int conditional move in this case.
> + /* Swap the comparison if isel can't handle it directly. Don't generate int
> + cmoves if we can't swap the condition code due to NaNs. */
"swap" has a specific meaning for comparisons, and this isn't it (it
refers to swapping the arguments).
You can do the reverse condition for all codes that include UN just
fine. reversed_comparison knows how to do this.
> + enum rtx_code op_code = GET_CODE (op);
> + if (op_code != LT && op_code != GT && op_code != LTU && op_code != GTU
> + && op_code != EQ)
There are functions to test this. Perhaps scc_comparison_operator
and exclude unordered? But, this seems wrong, as said.
> - switch (cond_code)
> - {
> - case LT: case GT: case LTU: case GTU: case EQ:
> - /* isel handles these directly. */
> - break;
Ah, you got that from existing code. Well, a good chance to improve
things, isn't it :-)
> new file mode 100644
> index 00000000000..d1bfab23482
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/ppc-fortran/pr104254.f90
> @@ -0,0 +1,25 @@
> +! { dg-do compile }
> +! { dg-require-effective-target powerpc_p9vector_ok }
> +! { dg-options "-mdejagnu-cpu=power9 -O1 -fnon-call-exceptions" }
> +
> +! PR target/104254. GCC would raise an assertion error if this program was
PR104256.
Segher
On Thu, Feb 17, 2022 at 05:38:07PM -0600, Segher Boessenkool wrote:
> Hi!
>
> First, you need to adjust after Robin's patch, and retest.
Robin's patch has the effct making rs6000_emit_int_cmove return false for
floating point comparisons, so I marked the bug as being a duplicate of PR
target/104335.
Hi,
> Robin's patch has the effct making rs6000_emit_int_cmove return false for
> floating point comparisons, so I marked the bug as being a duplicate of PR
> target/104335.
Didn't I just return false for MODE_CC? This should not affect proper
floating-point comparisons. It looks like the problem was indeed caused
by my original patch but then I wonder why I did not catch it in the
first place despite running a Power9 bootstrap and regtest (with Fortran
of course) that looked unchanged.
Shouldn't this have come up? I vaguely recall seeing maxloc FAILs at
some point but not in the final runs. Going to re-check because this
would have helped not introduce the problem that late.
Regards
Robin
On Thu, Feb 24, 2022 at 08:07:28AM +0100, Robin Dapp wrote:
> Hi,
>
> > Robin's patch has the effct making rs6000_emit_int_cmove return false for
> > floating point comparisons, so I marked the bug as being a duplicate of PR
> > target/104335.
>
> Didn't I just return false for MODE_CC? This should not affect proper
> floating-point comparisons. It looks like the problem was indeed caused
> by my original patch but then I wonder why I did not catch it in the
> first place despite running a Power9 bootstrap and regtest (with Fortran
> of course) that looked unchanged.
It only showed up with some specific options (-O1 -fnon-call-exceptions and
-mcpu set to power9 or power10). If you use -O0, -O2, or -O3 it doesn't show
up, and if you don't use -fnon-call-exceptions it doesn't show up. In
particular, you needed ISEL enabled (power8 doesn't enable it due to
performance reasons). Bill Seuer reported the bug. Maybe he has more details.
Returning false is fine in this case. It just says that we can't do
conditional move (i.e. use the ISEL instruction). The compiler will fall back
to doing a jump around a move.
However, in doing the patch, I tried to get the compiler to generate cmoves for
integer values with floating point conditions, and I just don't see it normally
being generated.
I suspect if we want to do it, it will be a much larger project for the GCC 13
time frame. As I recall, there was one Spec test that really wanted to do
something like this, but to re-architect this will be a large undertaking.
At the moment, we allow mixed floating point test and conditional move:
float f1, f2;
double d1, d2, d3;
d1 = (f1 == f2) ? d2 : d3;
When I added the IEEE 128-bit support, it because a combinatorial explosion to
try and handle all possible compares and moves. Eventually, we decided just to
only do the IEEE 128-bit cmove if the test and values being moved were the same
mode (i.e. KFmode or TFmode), and you couldn't mix float/double and __float128.
Int cmoves have traditionally just not thought of having floating point
comparisons. They could, it just wasn't thought of.
> Shouldn't this have come up? I vaguely recall seeing maxloc FAILs at
> some point but not in the final runs. Going to re-check because this
> would have helped not introduce the problem that late.
As I said, it needed some specific cases to get it to fail.
@@ -16174,18 +16174,35 @@ rs6000_emit_int_cmove (rtx dest, rtx op, rtx true_cond, rtx false_cond)
{
rtx condition_rtx, cr;
machine_mode mode = GET_MODE (dest);
- enum rtx_code cond_code;
rtx (*isel_func) (rtx, rtx, rtx, rtx, rtx);
bool signedp;
if (mode != SImode && (!TARGET_POWERPC64 || mode != DImode))
return false;
+ /* Swap the comparison if isel can't handle it directly. Don't generate int
+ cmoves if we can't swap the condition code due to NaNs. */
+ enum rtx_code op_code = GET_CODE (op);
+ if (op_code != LT && op_code != GT && op_code != LTU && op_code != GTU
+ && op_code != EQ)
+ {
+ if (!COMPARISON_P (op))
+ return false;
+
+ enum rtx_code rev_code = reverse_condition (op_code);
+ if (rev_code == UNKNOWN)
+ return false;
+
+ std::swap (false_cond, true_cond);
+ op = gen_rtx_fmt_ee (rev_code, GET_MODE (op),
+ XEXP (op, 0),
+ XEXP (op, 1));
+ }
+
/* We still have to do the compare, because isel doesn't do a
compare, it just looks at the CRx bits set by a previous compare
instruction. */
condition_rtx = rs6000_generate_compare (op, mode);
- cond_code = GET_CODE (condition_rtx);
cr = XEXP (condition_rtx, 0);
signedp = GET_MODE (cr) == CCmode;
@@ -16193,21 +16210,6 @@ rs6000_emit_int_cmove (rtx dest, rtx op, rtx true_cond, rtx false_cond)
? (signedp ? gen_isel_signed_si : gen_isel_unsigned_si)
: (signedp ? gen_isel_signed_di : gen_isel_unsigned_di));
- switch (cond_code)
- {
- case LT: case GT: case LTU: case GTU: case EQ:
- /* isel handles these directly. */
- break;
-
- default:
- /* We need to swap the sense of the comparison. */
- {
- std::swap (false_cond, true_cond);
- PUT_CODE (condition_rtx, reverse_condition (cond_code));
- }
- break;
- }
-
false_cond = force_reg (mode, false_cond);
if (true_cond != const0_rtx)
true_cond = force_reg (mode, true_cond);
new file mode 100644
@@ -0,0 +1,25 @@
+! { dg-do compile }
+! { dg-require-effective-target powerpc_p9vector_ok }
+! { dg-options "-mdejagnu-cpu=power9 -O1 -fnon-call-exceptions" }
+
+! PR target/104254. GCC would raise an assertion error if this program was
+! compiled with -O1 and -fnon-call-exceptions on a power9 or higher. The issue
+! occurs because at this optimization level, the compiler is trying to make
+! a conditional move to store integers using a 32-bit floating point compare.
+! It wants to use UNLE, which is not supported for integer modes.
+
+ real :: a(2), nan
+ real, allocatable :: c(:)
+ integer :: ia(1)
+
+ nan = 0.0
+ nan = 0.0/nan
+
+ a(:) = nan
+ ia = maxloc (a)
+ if (ia(1).ne.1) STOP 1
+
+ allocate (c(1))
+ c(:) = nan
+ deallocate (c)
+end