[Bug target/95740] New: Failure to avoid using the stack when interpreting a float as an integer when it is modified afterwards

[Bug target/95740] New: Failure to avoid using the stack when interpreting a float as an integer when it is modified afterwards

[gabravier at gmail dot com]

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

            Bug ID: 95740
           Summary: Failure to avoid using the stack when interpreting a
                    float as an integer when it is modified afterwards
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gabravier at gmail dot com
  Target Milestone: ---

int32_t f(float p)
{
    int32_t tmp;
    memcpy(&tmp, &p, sizeof(float));
    ++tmp;
    return tmp;
}

With -O3, LLVM outputs this :

f(float):
  movd eax, xmm0
  add eax, 1
  ret

GCC outputs this :

f(float):
  movd DWORD PTR [rsp-4], xmm0
  mov eax, 1
  add eax, DWORD PTR [rsp-4]
  ret

[Bug target/95740] Failure to avoid using the stack when interpreting a float as an integer when it is modified afterwards

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-06-18
           Keywords|                            |ra

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
;; return _1;

(insn 6 5 7 (parallel [
            (set (reg:SI 86)
                (plus:SI (subreg:SI (reg/v:SF 85 [ p ]) 0)
                    (const_int 1 [0x1])))
            (clobber (reg:CC 17 flags))
        ]) "t.c":5:5 -1
     (nil))

(insn 7 6 8 (set (reg:SI 84 [ <retval> ])
        (reg:SI 86)) "t.c":6:12 -1
     (nil))


Initial RTL couldn't be better...  but then LRA chooses to spill rather
than try a direct xmm->grp move.  -mtune=intel didn't help here (for
generic we IIRC avoid such moves)

[Bug target/95740] Failure to avoid using the stack when interpreting a float as an integer when it is modified afterwards

[crazylht at gmail dot com]

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

Hongtao.liu <crazylht at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |crazylht at gmail dot com

--- Comment #2 from Hongtao.liu <crazylht at gmail dot com> ---
Increase constraints preference and reduce sse->integer move cost can't help
it.

----
modified   gcc/config/i386/i386.md                                              
@@ -2294,9 +2294,9 @@                                                           

 (define_insn "*movsi_internal"                                                 
   [(set (match_operand:SI 0 "nonimmediate_operand"                             
-    "=r,m ,*y,*y,?*y,?m,?r,?*y,*v,*v,*v,m ,?r,?*v,*k,*k ,*rm,*k")              
+    "=r,m ,*y,*y,?*y,?m,?r,?*y,*v,*v,*v,r ,m,?*v,*k,*k ,*rm,*k")               
         (match_operand:SI 1 "general_operand"                                  
-    "g ,re,C ,*y,m  ,*y,*y,r  ,C ,*v,m ,*v,*v,r  ,*r,*km,*k ,CBC"))]           
+    "g ,re,C ,*y,m  ,*y,*y,r  ,C ,*v,m ,v,*v,r  ,*r,*km,*k ,CBC"))]            
   "!(MEM_P (operands[0]) && MEM_P (operands[1]))"                              
 {                                                                              
   switch (get_attr_type (insn))                                                
modified   gcc/config/i386/x86-tune-costs.h                                     
@@ -1624,7 +1624,7 @@ struct processor_costs skylake_cost = {                   
                                            in 32,64,128,256 and 512-bit */     
   {8, 8, 8, 12, 24},                   /* cost of storing SSE registers        
                                            in 32,64,128,256 and 512-bit */     
-  6, 6,                                        /* SSE->integer and
integer->SSE moves */                                                           
+  2, 2,                                        /* SSE->integer and
integer->SSE moves */  

------

It seems to me for insn inserted reloaded before

18: r89:SI=r87:SF#0

(insn 18 16 6 2 (set (reg:SI 89) 
 (subreg:SI (reg:SF 87) 0))

LRA prefer to put reg:SI 89 into memory since it would be used later.

[Bug target/95740] Failure to avoid using the stack when interpreting a float as an integer when it is modified afterwards

[gabravier at gmail dot com]

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

--- Comment #3 from Gabriel Ravier <gabravier at gmail dot com> ---
I've also encountered what looks like a duplicate of this bug, although I'm not
sure but it seems likely:

int foo(float f)
{
  union
  {
    float f;
    int i;
  } z = { .f = f };

  return z.i - 1;
}

Which outputs roughly the same assembly code as the initial test case.

[Bug target/95740] Failure to avoid using the stack when interpreting a float as an integer when it is modified afterwards

[crazylht at gmail dot com]

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

--- Comment #4 from Hongtao.liu <crazylht at gmail dot com> ---
It can be fixed by

2 files changed, 4 insertions(+), 2 deletions(-)
gcc/config/i386/i386.c | 2 +-
gcc/config/i386/i386.h | 4 +++-

modified   gcc/config/i386/i386.c
@@ -19194,7 +19194,7 @@ ix86_preferred_reload_class (rtx x, reg_class_t
regclass)

   /* Prefer SSE regs only, if we can use them for math.  */
   if (SSE_FLOAT_MODE_P (mode) && TARGET_SSE_MATH)
-    return SSE_CLASS_P (regclass) ? regclass : NO_REGS;
+    return INT_SSE_CLASS_P (regclass) ? regclass : NO_REGS;

   /* Generally when we see PLUS here, it's the function invariant
      (plus soft-fp const_int).  Which can only be computed into general
modified   gcc/config/i386/i386.h
@@ -1283,7 +1283,9 @@ enum reg_class
   reg_class_subset_p ((CLASS), FLOAT_REGS)
 #define SSE_CLASS_P(CLASS) \
   reg_class_subset_p ((CLASS), ALL_SSE_REGS)
-#define MMX_CLASS_P(CLASS) \
+#define INT_SSE_CLASS_P(CLASS) \
+  reg_class_subset_p ((CLASS), INT_SSE_REGS)
+#define MMX_CLASS_P(CLASS)                     \
   ((CLASS) == MMX_REGS)
 #define MASK_CLASS_P(CLASS) \
   reg_class_subset_p ((CLASS), ALL_MASK_REGS)

I'm testing performance impact.

[Bug target/95740] Failure to avoid using the stack when interpreting a float as an integer when it is modified afterwards

[crazylht at gmail dot com]

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

--- Comment #5 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Hongtao.liu from comment #4)
> It can be fixed by
> 
> 2 files changed, 4 insertions(+), 2 deletions(-)
> gcc/config/i386/i386.c | 2 +-
> gcc/config/i386/i386.h | 4 +++-
> 
> modified   gcc/config/i386/i386.c
> @@ -19194,7 +19194,7 @@ ix86_preferred_reload_class (rtx x, reg_class_t
> regclass)
>  
>    /* Prefer SSE regs only, if we can use them for math.  */
>    if (SSE_FLOAT_MODE_P (mode) && TARGET_SSE_MATH)
> -    return SSE_CLASS_P (regclass) ? regclass : NO_REGS;
> +    return INT_SSE_CLASS_P (regclass) ? regclass : NO_REGS;
>  
>    /* Generally when we see PLUS here, it's the function invariant
>       (plus soft-fp const_int).  Which can only be computed into general
> modified   gcc/config/i386/i386.h
> @@ -1283,7 +1283,9 @@ enum reg_class
>    reg_class_subset_p ((CLASS), FLOAT_REGS)
>  #define SSE_CLASS_P(CLASS) \
>    reg_class_subset_p ((CLASS), ALL_SSE_REGS)
> -#define MMX_CLASS_P(CLASS) \
> +#define INT_SSE_CLASS_P(CLASS) \
> +  reg_class_subset_p ((CLASS), INT_SSE_REGS)
> +#define MMX_CLASS_P(CLASS)			\
>    ((CLASS) == MMX_REGS)
>  #define MASK_CLASS_P(CLASS) \
>    reg_class_subset_p ((CLASS), ALL_MASK_REGS)
> 
> I'm testing performance impact.

orginally,  ix86_preferred_reload_class (SFmode, GENERAL_REGS) return NO_REG,
and it increase reject by LRA_MAX_REJECT(600) which make it impossible to
reload to general_regs.

---------------------cut from lra-constraints.c------------------------
              /* Check strong discouragement of reload of non-constant
                 into class THIS_ALTERNATIVE.  */
              if (! CONSTANT_P (op) && ! no_regs_p
                  && (targetm.preferred_reload_class
                      (op, this_alternative) == NO_REGS
                      || (curr_static_id->operand[nop].type == OP_OUT
                          && (targetm.preferred_output_reload_class
                              (op, this_alternative) == NO_REGS))))
                {
                  if (offmemok && REG_P (op))
                    {
                      if (lra_dump_file != NULL)
                        fprintf
                          (lra_dump_file,
                           "            %d Spill pseudo into memory:
reject+=3\n",
                           nop);
                      reject += 3;
                    }
                  else
                    {
                      if (lra_dump_file != NULL)
                        fprintf
                          (lra_dump_file,
                           "            %d Non-prefered reload: reject+=%d\n",
                           nop, LRA_MAX_REJECT);
                      reject += LRA_MAX_REJECT;
                    }
                }
-------------------------------------------------------

with gcc -O2 test.c -mfpmath=387, trunk gcc can also generate 

foo(float):
        movd    %xmm0, %eax
        addl    $1, %eax
        ret

[Bug target/95740] Failure to avoid using the stack when interpreting a float as an integer when it is modified afterwards

[cvs-commit at gcc dot gnu.org]

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

--- 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:d1011a41efd121ede2f427c1dffd6ac62f92962e

commit r12-5800-gd1011a41efd121ede2f427c1dffd6ac62f92962e
Author: liuhongt <hongtao.liu@intel.com>
Date:   Tue Nov 30 13:50:11 2021 +0800

    Prefer INT_SSE_REGS for SSE_FLOAT_MODE_P in preferred_reload_class.

    When moves between integer and sse registers are cheap.

    2021-12-06  Hongtao Liu  <Hongtao.liu@intel.com>
                Uroš Bizjak  <ubizjak@gmail.com>
    gcc/ChangeLog:

            PR target/95740
            * config/i386/i386.c (ix86_preferred_reload_class): Allow
            integer regs when moves between register units are cheap.
            * config/i386/i386.h (INT_SSE_CLASS_P): New.

    gcc/testsuite/ChangeLog:

            * gcc.target/i386/pr95740.c: New test.

[Bug target/95740] Failure to avoid using the stack when interpreting a float as an integer when it is modified afterwards

[crazylht at gmail dot com]

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

--- Comment #7 from Hongtao.liu <crazylht at gmail dot com> ---
Fixed in GCC12.

[Bug target/95740] Failure to avoid using the stack when interpreting a float as an integer when it is modified afterwards

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
as mentioned fixed in GCC 12.