On 2/7/25 4:02 PM, Michael Meissner wrote:
> (define_predicate "invert_fpmask_comparison_operator"
> - (match_code "ne,unlt,unle"))
> + (ior (match_code "ne")
> + (and (match_code "unlt,unle")
> + (match_test "!HONOR_NANS (DFmode) || !TARGET_P9_VECTOR"))))
Is it always safe to use DFmode here in the HONOR_NANS macro?
Meaning does it always give the same answer as using SFmode, TFmode,
IFmore and KFmode? Ditto for the other use of HONOR_NANS (DFmode).
> enum rtx_code
> -rs6000_reverse_condition (machine_mode mode, enum rtx_code code)
> +rs6000_reverse_condition (machine_mode mode,
> + enum rtx_code code,
> + bool no_ordered)
I'm not sure I'm a fan of the no_ordered name. Maybe use not_ordered
instead? Or maybe even better, rename it to "ordered" and modify the
code that uses it to handle the reversed meaning?
> /* Reversal of FP compares takes care -- an ordered compare
> - becomes an unordered compare and vice versa. */
> + becomes an unordered compare and vice versa.
> +
> + However, this is not safe for ordered comparisons (i.e. for isgreater,
> + etc.) starting with the power9 because ifcvt.cc will want to create a
> fp
> + cmove, and the x{s,v}cmp{eq,gt,ge}{dp,qp} instructions will trap if one
> of
> + the arguments is a signalling NaN. */
I think this could use a little work smithing and there is some stray
whitespace. You also explicitly mention signalling NaN, but the code
uses HONOR_NANS, not HONOR_SNANS. Is that intentional?
> /* Can the condition code MODE be safely reversed? This is safe in
> all cases on this port, because at present it doesn't use the
> - trapping FP comparisons (fcmpo). */
> + trapping FP comparisons (fcmpo).
> +
> + However, this is not safe for ordered comparisons (i.e. for isgreater,
> etc.)
> + starting with the power9 because ifcvt.cc will want to create a fp cmove,
> + and the x{s,v}cmp{eq,gt,ge}{dp,qp} instructions will trap if one of the
> + arguments is a signalling NaN. */
Likewise.
> +/* { dg-do compile } */
> +/* { dg-options "-mdejagnu-cpu=power9 -O2" } */
> +/* { dg-require-effective-target powerpc_vsx } */
> +
> +/* PR target/118541 says that the ordered comparison functions like isgreater
> + should not optimize floating point conditional moves to use
> + x{s,v}cmp{eq,gt,ge}{dp,qp} and xxsel since that instruction can cause
> traps
> + if one of the arguments is a signaling NaN. */
> +
> +/* Verify isgreater does not generate xscmpgtdp. */
[snip]
> +/* { dg-final { scan-assembler-times {\mxscmpg[te]dp\M} 1 } } */
> +/* { dg-final { scan-assembler-times {\mxxsel\M} 1 } } */
> +/* { dg-final { scan-assembler-times {\mxscmpudp\M|\mfcmpu\M} 1 } } */
I think this would be safer if we split this into two test cases, one with
each of the functions. I'm worried that if we were to somehow accidentally
swap the results of your new code, we'd still produce one each of the
instructions above and we wouldn't notice. I think it's safer to have one
test case for each function here (ordered and normal) and explicitly look
for the insns you want, while at the same time using scan-assembler-not for
the insns you don't want to see.
Peter
