[Bug c/105453] New: wrong choice of source in load instruction

[Bug c/105453] New: wrong choice of source in load instruction

[absoler at smail dot nju.edu.cn]

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

            Bug ID: 105453
           Summary: wrong choice of source in load instruction
           Product: gcc
           Version: 11.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: absoler at smail dot nju.edu.cn
  Target Milestone: ---

Created attachment 52917
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52917&action=edit
concurrent case

Here's the code:

int g_6[2] = {0x67D50F82L,0x67D50F82L};
int g_10 = 0L;

int func_1(void)
{ 
    int *l_9 = &g_10;
    (*l_9) = (g_6[0]);
    if (g_6[1])
    {
        return g_10;
    }
    return 1;
}

and on gcc-11.3.0 with -O1 option, the assembly code are as follows:

   0x000000000040114d <+0>:     mov    0x2eed(%rip),%eax        # 0x404040
<g_6>
   0x0000000000401153 <+6>:     mov    %eax,0x2ef3(%rip)        # 0x40404c
<g_10>
   0x0000000000401159 <+12>:    cmpl   $0x0,0x2ee4(%rip)        # 0x404044
<g_6+4>
   0x0000000000401160 <+19>:    mov    $0x1,%eax
   0x0000000000401165 <+24>:    cmovne 0x2ed4(%rip),%eax        # 0x404040
<g_6>
   0x000000000040116c <+31>:    retq

yes, it assert that g_10 = g_6[0] and this may cause problem in concurrent
environment. The case files uploaded can show that the generated code is the
same as above in an environment that g_6 may be modified by other thread.

[Bug c/105453] wrong choice of source in load instruction

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There is no issue here as there is no atomic barrier as required by the c11 and
c++11 standards in their memory model

[Bug c/105453] wrong choice of source in load instruction

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |---

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
.

[Bug rtl-optimization/105453] load introduced by ce1 for conditional loads at -O1, might cause issues with the C/C++ memory model

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|wrong choice of source in   |load introduced by ce1 for
                   |load instruction            |conditional loads at -O1,
                   |                            |might cause issues with the
                   |                            |C/C++ memory model

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The problem shows up in ce1. Someone who is more in tune with the C/C++ memory
model should figure out if adding an extra load here is valid or not.

I think it is valid still since there is no load barrier used.

[Bug rtl-optimization/105453] load introduced by ce1 for conditional loads at -O1, might cause issues with the C/C++ memory model

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-05-03
      Known to work|                            |12.0
           Keywords|                            |missed-optimization,
                   |                            |needs-bisection
             Status|UNCONFIRMED                 |NEW

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
With gcc 12 we get

_Z6func_1v:
.LFB0:
        .cfi_startproc
        movl    g_6(%rip), %eax
        movl    %eax, g_10(%rip)
        cmpl    $0, g_6+4(%rip)
        movl    $1, %edx
        cmove   %edx, %eax
        ret

the difference is that we somehow un-CSE the g_6 load at RTL expansion with GCC
11 and then things go downhill later:

;; Generating RTL for gimple basic block 2

;; _1 = g_6[0];

(insn 6 5 7 (set (reg/f:DI 84)
        (symbol_ref:DI ("g_6") [flags 0x2]  <var_decl 0x7ffff7ff3bd0 g_6>))
"t.c":7:20 -1
     (nil))

(insn 7 6 0 (set (reg:SI 83 [ <retval> ])
        (mem/c:SI (reg/f:DI 84) [1 g_6[0]+0 S4 A64])) "t.c":7:20 -1
     (nil))

;; g_10 = _1;

(insn 8 7 0 (set (mem/c:SI (symbol_ref:DI ("g_10") [flags 0x2]  <var_decl
0x7ffff7ff3c60 g_10>) [1 g_10+0 S4 A32])
        (reg:SI 83 [ <retval> ])) "t.c":7:12 -1
     (nil))

;; if (_2 != 0)

(insn 9 8 10 (set (reg/f:DI 85)
        (symbol_ref:DI ("g_6") [flags 0x2]  <var_decl 0x7ffff7ff3bd0 g_6>))
"t.c":8:14 -1
     (nil))

(insn 10 9 11 (set (reg:CCZ 17 flags)
        (compare:CCZ (mem/c:SI (plus:DI (reg/f:DI 85)
                    (const_int 4 [0x4])) [1 g_6[1]+0 S4 A32])
            (const_int 0 [0]))) "t.c":8:5 -1
     (nil))


the same happens with GCC 12.  CSE cleans this up so it's maybe not important
but in GCC 11 forwprop then does

     4: NOTE_INSN_BASIC_BLOCK 2
     2: NOTE_INSN_FUNCTION_BEG
-    6: r84:DI=`g_6'
-    7: r83:SI=[r84:DI]
-      REG_DEAD r84:DI
+    7: r83:SI=[`g_6']
     8: [`g_10']=r83:SI
-    9: r85:DI=`g_6'
-   10: flags:CCZ=cmp([r84:DI+0x4],0)
-      REG_DEAD r85:DI
+   10: flags:CCZ=cmp([const(`g_6'+0x4)],0)

which seems to confuse CE enough to emit the extra load:

     4: NOTE_INSN_BASIC_BLOCK 2
     2: NOTE_INSN_FUNCTION_BEG
     7: r83:SI=[`g_6']
     8: [`g_10']=r83:SI
-   10: flags:CCZ=cmp([const(`g_6'+0x4)],0)
-   11: pc={(flags:CCZ==0)?L24:pc}
-      REG_DEAD flags:CCZ
-      REG_BR_PROB 708669604
-      ; pc falls through to BB 4
-   24: L24:
-   23: NOTE_INSN_BASIC_BLOCK 3
-    3: r83:SI=0x1
-   17: L17:
-   20: NOTE_INSN_BASIC_BLOCK 4
+   25: r88:SI=[`g_6']
+   26: r87:SI=0x1
+   27: flags:CCZ=cmp([const(`g_6'+0x4)],0)
+   28: r83:SI={(flags:CCZ==0)?r87:SI:r88:SI}
    18: ax:SI=r83:SI

disabling fwprop1 produces

+    6: r84:DI=`g_6'
+    7: r83:SI=[r84:DI]
+      REG_DEAD r84:DI
+    8: [`g_10']=r83:SI
+   26: r87:SI=0x1
+   25: flags:CCZ=cmp([r84:DI+0x4],0)
+   27: r83:SI={(flags:CCZ!=0)?r83:SI:r87:SI}
    18: ax:SI=r83:SI

again.  Likely the new SSA based forwprop "fixed" this, but maybe only
accidentially.  On trunk fwprop1 does

-    6: r84:DI=`g_6'
-    7: r83:SI=[r84:DI]
-      REG_DEAD r84:DI
+    7: r83:SI=[`g_6']
     8: [`g_10']=r83:SI
-    9: r85:DI=`g_6'
-   10: flags:CCZ=cmp([r84:DI+0x4],0)
-      REG_DEAD r85:DI
+   10: flags:CCZ=cmp([const(`g_6'+0x4)],0)

[Bug rtl-optimization/105453] [9/10/11 Regression] load introduced by ce1 for conditional loads at -O1, might cause issues with the C/C++ memory model

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |7.5.0
            Summary|load introduced by ce1 for  |[9/10/11 Regression] load
                   |conditional loads at -O1,   |introduced by ce1 for
                   |might cause issues with the |conditional loads at -O1,
                   |C/C++ memory model          |might cause issues with the
                   |                            |C/C++ memory model
   Target Milestone|---                         |9.5
      Known to fail|                            |11.3.1, 9.4.1

[Bug rtl-optimization/105453] [9/10/11 Regression] load introduced by ce1 for conditional loads at -O1, might cause issues with the C/C++ memory model

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #1)
> There is no issue here as there is no atomic barrier as required by the c11
> and c++11 standards in their memory model

I agree that GCC's output is allowed by the memory model. If another thread is
changing either of those globals then func_1 needs to be synchronised with it
to avoid data races, i.e. it would have undefined behaviour anyway, whatever
code GCC generated.

[Bug rtl-optimization/105453] [10/11 Regression] load introduced by ce1 for conditional loads at -O1, might cause issues with the C/C++ memory model

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.5                         |10.4

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9 branch is being closed

[Bug rtl-optimization/105453] [10/11 Regression] load introduced by ce1 for conditional loads at -O1, might cause issues with the C/C++ memory model

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug rtl-optimization/105453] [11 Regression] load introduced by ce1 for conditional loads at -O1, might cause issues with the C/C++ memory model

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.