[Bug libstdc++/98612] New: _mm_comieq_sd has wrong semantics

[Bug libstdc++/98612] New: _mm_comieq_sd has wrong semantics

[guillaume.piolat at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98612

            Bug ID: 98612
           Summary: _mm_comieq_sd has wrong semantics
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: guillaume.piolat at gmail dot com
  Target Milestone: ---

Created attachment 49926
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49926&action=edit
Behaviour with 3 compilers

_mm_comieq_sd has different NaN semantics for different people.

# The Unordered team
- GCC will return 1 for a comparison that involved NaN.
- this maps to the underlying instruction

# The Ordered team
- Intel intrinsics guide says: 

     RETURN ( a[63:0] == b[63:0] ) ? 1 : 0

  which indicates an ordered comparison.

- ICC and clang return 0 for a NaN comparison (see image attachments)


As the x86 "intrinsics" are meant as a portable API (vs "builtins"), I would
suggest it is expected that the same result than clang and ICC is returned.

GCC has the same problem for other intrinsics that don't actually map to the
instruction semantics, such as:

sse2:
  - _mm_comieq_sd
  - _mm_comile_sd
  - _mm_comilt_sd
  - _mm_comineq_sd (instruction is an ordered comparison, but intrinsics is an
unordered comparison)

sse:
  - _mm_comieq_ss
  - _mm_comile_ss
  - _mm_comilt_ss
  - _mm_comineq_ss

I don't know for later instruction sets.

[Bug libstdc++/98612] _mm_comieq_sd has wrong semantics

[guillaume.piolat at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98612

--- Comment #1 from Guillaume Piolat <guillaume.piolat at gmail dot com> ---
Illustration of the issue for _mm_comineq_sd
=> https://d.godbolt.org/z/zn51Th

[Bug target/98612] _mm_comieq_sd has wrong semantics

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98612

--- Comment #2 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Guillaume Piolat from comment #0)
> Created attachment 49926 [details]
> Behaviour with 3 compilers
> 
> _mm_comieq_sd has different NaN semantics for different people.
> 
> # The Unordered team
> - GCC will return 1 for a comparison that involved NaN.
> - this maps to the underlying instruction
> 
> # The Ordered team
> - Intel intrinsics guide says: 
> 
>      RETURN ( a[63:0] == b[63:0] ) ? 1 : 0
> 
>   which indicates an ordered comparison.

ICC take _mm_{u,}comi{eq,lt,le,gt,ge}_s{s,d} as ordered comparison, and
_mm_{u,}comineq_s{s,d} as unordered comparison, GCC didn't take {Q,}NAN operand
into consideration.

The codes has been in gcc for more than 15 years, and I'm not sure if some
applications are taking advantage of this "bug" in gcc, so do we really need to
"fix" it?

Technically, we can just "refine" as.

modified   gcc/config/i386/emmintrin.h
@@ -515,7 +515,7 @@ _mm_cmpunord_sd (__m128d __A, __m128d __B)
 extern __inline int __attribute__((__gnu_inline__, __always_inline__,
__artificial__))
 _mm_comieq_sd (__m128d __A, __m128d __B)
 {
-  return __builtin_ia32_comisdeq ((__v2df)__A, (__v2df)__B);
+  return __A[0] == __B[0];
 }

[Bug target/98612] _mm_comieq_sd has wrong semantics

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98612

--- Comment #3 from Hongtao.liu <crazylht at gmail dot com> ---
Also found some dead code in ix86_expand_sse_comi which is called by
   if (fcode >= IX86_BUILTIN__BDESC_COMI_FIRST
      && fcode <= IX86_BUILTIN__BDESC_COMI_LAST)
where all builtins are defined with flags as 0.


2 files changed, 9 deletions(-)
gcc/config/i386/i386-builtins.h | 4 ----
gcc/config/i386/i386-expand.c   | 5 -----

modified   gcc/config/i386/i386-builtins.h
@@ -236,10 +236,6 @@ struct builtin_isa {

 /* Bits for builtin_description.flag.  */

-/* Set when we don't support the comparison natively, and should
-   swap_comparison in order to support it.  */
-#define BUILTIN_DESC_SWAP_OPERANDS     1
-
 struct builtin_description
 {
   const HOST_WIDE_INT mask;
modified   gcc/config/i386/i386-expand.c
@@ -8634,11 +8634,6 @@ ix86_expand_sse_comi (const struct builtin_description
*d, tree exp,
   if (VECTOR_MODE_P (mode1))
     op1 = safe_vector_operand (op1, mode1);

-  /* Swap operands if we have a comparison that isn't available in
-     hardware.  */
-  if (d->flag & BUILTIN_DESC_SWAP_OPERANDS)
-    std::swap (op0, op1);
-
   target = gen_reg_rtx (SImode);
   emit_move_insn (target, const0_rtx);
   target = gen_rtx_SUBREG (QImode, target, 0);

[back]

[Bug target/98612] _mm_comieq_sd has wrong semantics

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98612

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
                 CC|                            |uros at gcc dot gnu.org

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
I guess some people might think intrinsics map to the underlying instruction
...
to me the intrinsics guide isn't sufficiently clear on the semantics.

[Bug target/98612] _mm_comieq_sd has wrong semantics

[guillaume.piolat at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98612

--- Comment #5 from Guillaume Piolat <guillaume.piolat at gmail dot com> ---
My reasoning for reporting (while it is minor) is the following:
- it might not be a conscious choice from GCC developers
- this is the only intrinsics I've found in mmx/sse/sse2 for which GCC has
different semantics than ICC and clang. 
- clang wiki define intel intrinsics as the "portable API" between compilers
- people will copy/paste intel intrinsics code between compilers. So fixing it
is breaking, but leaving it is also breaking
- in any case it is surprising

[Bug target/98612] _mm_comieq_sd has wrong semantics

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98612

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by hongtao Liu <liuhongt@gcc.gnu.org>:

https://gcc.gnu.org/g:240f0a490dae0fb9ef72fa21a93e8088d17fb682

commit r11-6605-g240f0a490dae0fb9ef72fa21a93e8088d17fb682
Author: liuhongt <hongtao.liu@intel.com>
Date:   Mon Jan 11 14:47:49 2021 +0800

    Delete dead code in ix86_expand_sse_comi.

    d->flag is always 0 for builtins located in
    BDESC_FIRST (comi,COMI,...)
    ...
    BDESC_END (COMI, PCMPESTR)

    gcc/ChangeLog:
            PR target/98612
            * config/i386/i386-builtins.h (BUILTIN_DESC_SWAP_OPERANDS):
            Deleted.
            * config/i386/i386-expand.c (ix86_expand_sse_comi): Delete
            dead code.

[Bug target/98612] _mm_comieq_sd has wrong semantics

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98612

--- Comment #7 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Guillaume Piolat from comment #5)
> My reasoning for reporting (while it is minor) is the following:
> - it might not be a conscious choice from GCC developers
> - this is the only intrinsics I've found in mmx/sse/sse2 for which GCC has
> different semantics than ICC and clang. 
> - clang wiki define intel intrinsics as the "portable API" between compilers
> - people will copy/paste intel intrinsics code between compilers. So fixing
> it is breaking, but leaving it is also breaking
> - in any case it is surprising

I asked my colleagues within intel to revise the descriptions in the intrinsics
guide to make it more explicit about NAN operands.

I'll fix this issue after the update of intrinsics guide.

[Bug target/98612] _mm_comieq_sd has wrong semantics

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98612

--- Comment #8 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Hongtao.liu from comment #7)
> I asked my colleagues within intel to revise the descriptions in the
> intrinsics guide to make it more explicit about NAN operands.
> 
> I'll fix this issue after the update of intrinsics guide.

BTW: To get NaN semantics, one can always use relational operation instead of a
builtin, so I'd say that builtin should correspond to a naked instruction (so,
in the way GCC does).

[Bug target/98612] _mm_comieq_sd has wrong semantics

[andysem at mail dot ru]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98612

andysem at mail dot ru changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andysem at mail dot ru

--- Comment #9 from andysem at mail dot ru ---
Some relevant discussion here:

https://stackoverflow.com/questions/75818896/mm-comieq-ss-difference-between-clang-and-gcc

Indeed, the description for _mm_comieq_ss in the Intrinsics Guide 3.6.5 as of
2022-01-24 takes NaNs into account:

https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=comiss&ig_expand=1424

I think, the point that the intrinsic should correspond to the naked
instruction is not correct in this case. The intrinsic corresponds to the same
instruction either way, the question is only which flags are considered the
result of the intrinsic. And it seems that the intended behavior (as indicated
by the updated docs, icc and clang) is to check ZF & PF, not just ZF as gcc is
currently doing.