On Fri, Feb 07, 2025 at 05:51:19PM -0600, Peter Bergner wrote:
> 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).
The problem is we do not have the original floating point mode. The mode in
question is CCFPmode, i.e. the mode used for setting the CR registers. The
code originally just used !flag_finite_math. I could go back to that, I
thought HONOR_NANS was clearer.
> > 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?
I can do that, but I would problably named it something like
ordered_compare_ok. Just ordered by itself would say to me that only ordered
comparisons are allowed, when the majority of comparisons that the function
sees are the normal unordered comparisons (i.e. EQ, NE, GT, GE, LE, and LT).
> > /* 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?
As I said in my previous reply, it is intentional. The option -fsignaling-nans
is not normally set. However, I felt that if the user explicitly used one of
the ordered comparisons (i.e. isgreater in this case) that the compiler should
explicitly generate code that is safe for using signalling NaNs because that is
how those functions are describe.
If the original user (and the library writers, etc.) all use -fsingaling-nans,
then I would be happy to change it to HONOR_SNAN.
Or we can just go back to using !flag_finite_math.
> > +/* { 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.
Sounds reasonable.
--
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meiss...@linux.ibm.com
