[Bug rtl-optimization/102306] New: Volatile pointer dereferenced twice

[Bug rtl-optimization/102306] New: Volatile pointer dereferenced twice

[cederman at gaisler dot com]

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

            Bug ID: 102306
           Summary: Volatile pointer dereferenced twice
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: cederman at gaisler dot com
                CC: ebotcazou at libertysurf dot fr, segher at kernel dot crashing.org
  Target Milestone: ---

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

The following code (full case in attachment) generates two loads from the
volatile "a" pointer on SPARC:

C
  char b = *(volatile unsigned char *)a;
  if (!b)
    return;

Asm
  sethi   %hi(a), %g1
  ld      [%g1+%lo(a)], %g1
  ldub    [%g1], %g2
  ldub    [%g1], %g1
  cmp     %g1, 0

The code is intended to read a memory mapped register that will change after a
read, so two reads will give the wrong value to the comparison.

A bisect showed that this started to happen after "c4c5ad1d6d1e1e1fe7a:
combine: Allow combining two insns to two insns".

The compiler (on commit fc4a29c0781186269dc, latest master at the time) was
configured with:

configure --target=sparc-gaisler-elf --enable-languages=c --disable-nls
--disable-libmudflap --disable-libssp --enable-version-specific-runtime-libs
--disable-fixed-point --disable-decimal-float --disable-shared

and the test was compiled with:

sparc-gaisler-elf -mcpu=v8 -O2

I do not know if this is a generic problem or a SPARC specific one.

[Bug target/102306] Volatile pointer dereferenced twice

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |sparc-elf
             Status|UNCONFIRMED                 |NEW
           Keywords|                            |wrong-code
          Component|rtl-optimization            |target
   Last reconfirmed|                            |2021-09-13
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I would think it's sparc specific for now.  Confirmed with simplified

void c(void);
void test(volatile unsigned char * a) 
{ 
  char b = *a;
  if (!b)
    return;
  if (b & 2)
    c();
}

[Bug rtl-optimization/102306] Volatile pointer dereferenced twice

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|target                      |rtl-optimization

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
But it's definitely combine duplicating the volatile MEM:

-    6: r109:QI=[r111:SI]
-      REG_DEAD r111:SI
-    7: r112:SI=zero_extend(r109:QI)
+    6: r109:QI=[%i0:SI]
+    7: r112:SI=zero_extend([%i0:SI])
+      REG_DEAD %i0:SI

[Bug rtl-optimization/102306] Volatile pointer dereferenced twice

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu.org

--- Comment #3 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Trying 6 -> 7:
    6: r109:QI=[%i0:SI]
      REG_DEAD %i0:SI
    7: r112:SI=zero_extend(r109:QI)
Failed to match this instruction:
(parallel [
        (set (reg:SI 112 [ _1+-3 ])
            (zero_extend:SI (mem/v:QI (reg:SI 24 %i0 [ a ]) [0 *a_5(D)+0 S1
A8])))
        (set (reg:QI 109 [ _1 ])
            (mem/v:QI (reg:SI 24 %i0 [ a ]) [0 *a_5(D)+0 S1 A8]))
    ])
Failed to match this instruction:
(parallel [
        (set (reg:SI 112 [ _1+-3 ])
            (zero_extend:SI (mem/v:QI (reg:SI 24 %i0 [ a ]) [0 *a_5(D)+0 S1
A8])))
        (set (reg:QI 109 [ _1 ])
            (mem/v:QI (reg:SI 24 %i0 [ a ]) [0 *a_5(D)+0 S1 A8]))
    ])
Successfully matched this instruction:
(set (reg:QI 109 [ _1 ])
    (mem/v:QI (reg:SI 24 %i0 [ a ]) [0 *a_5(D)+0 S1 A8]))
Successfully matched this instruction:
(set (reg:SI 112 [ _1+-3 ])
    (zero_extend:SI (mem/v:QI (reg:SI 24 %i0 [ a ]) [0 *a_5(D)+0 S1 A8])))
allowing combination of insns 6 and 7
original costs 4 + 4 = 8
replacement costs 4 + 4 = 8
modifying insn i2     6: r109:QI=[%i0:SI]
deferring rescan insn with uid = 6.
modifying insn i3     7: r112:SI=zero_extend([%i0:SI])
      REG_DEAD %i0:SI
deferring rescan insn with uid = 7.

This should probably have been prevented in two different ways: 1) the volatile
load and 2) the equal cost for a 2->2 combination.

[Bug rtl-optimization/102306] Volatile pointer dereferenced twice

[ebotcazou at gcc dot gnu.org]

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

--- Comment #4 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
There is substantial checking for volatile references in can_combine_p but it
implicitly assumes that the combination reduces the number of instructions.

[Bug rtl-optimization/102306] Volatile pointer dereferenced twice

[ebotcazou at gcc dot gnu.org]

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

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Created attachment 51460
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51460&action=edit
Tentative fix

To be tested.

[Bug rtl-optimization/102306] [9/10/11/12 Regression] Volatile pointer dereferenced twice

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |9.5
            Summary|Volatile pointer            |[9/10/11/12 Regression]
                   |dereferenced twice          |Volatile pointer
                   |                            |dereferenced twice

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Just so it is easier for the future the commit was r9-2064-gc4c5ad1d6d1e1e1fe7a
.

[Bug rtl-optimization/102306] [9/10/11/12 Regression] Volatile pointer dereferenced twice

[segher at gcc dot gnu.org]

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

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |segher at gcc dot gnu.org

--- Comment #7 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Eric Botcazou from comment #3)
> This should probably have been prevented in two different ways: 1) the
> volatile load and 2) the equal cost for a 2->2 combination.

No, it is equal cost, but fewer dependencies; it is lower latency.  This is
how all costing in combine works.

1) of course *is* a problem.

[Bug rtl-optimization/102306] [9/10/11/12 Regression] Volatile pointer dereferenced twice

[segher at gcc dot gnu.org]

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

--- Comment #8 from Segher Boessenkool <segher at gcc dot gnu.org> ---
The patch in c#5 looks good.  Since you test added_sets_0 and added_sets_1
as well, does this mean it could happen before this patch as well?

We already did have some things that did combine 2->2 (via a splitter),
fwiw.  But we do not check everything from splitters, we just trust it is
good.

[Bug rtl-optimization/102306] [9/10/11/12 Regression] Volatile pointer dereferenced twice

[ebotcazou at gcc dot gnu.org]

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

--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> The patch in c#5 looks good.  Since you test added_sets_0 and added_sets_1
> as well, does this mean it could happen before this patch as well?

No idea, but it seems strange to disallow the copy for i2 and not the others.

[Bug rtl-optimization/102306] [9/10/11/12 Regression] Volatile pointer dereferenced twice

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|ebotcazou at gcc dot gnu.org       |
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |ebotcazou at gcc dot gnu.org

[Bug rtl-optimization/102306] [9/10/11/12 Regression] Volatile pointer dereferenced twice

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:687e30d9d74f5c69a54617147ce3433b07ee59ce

commit r12-3611-g687e30d9d74f5c69a54617147ce3433b07ee59ce
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Fri Sep 17 10:12:12 2021 +0200

    Fix PR rtl-optimization/102306

    This is a duplication of volatile loads introduced during GCC 9 development
    by the 2->2 mechanism of the RTL combiner.  There is already a substantial
    checking for volatile references in can_combine_p but it implicitly assumes
    that the combination reduces the number of instructions, which is of course
    not the case here.  So the fix teaches try_combine to abort the combination
    when it is about to make a copy of volatile references to preserve them.

    gcc/
            PR rtl-optimization/102306
            * combine.c (try_combine): Abort the combination if we are about to
            duplicate volatile references.

    gcc/testsuite/
            * gcc.target/sparc/20210917-1.c: New test.

[Bug rtl-optimization/102306] [9/10/11/12 Regression] Volatile pointer dereferenced twice

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Eric Botcazou
<ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:469aad8b5d787c728fbe8b4ec6537385d7815d5a

commit r11-9011-g469aad8b5d787c728fbe8b4ec6537385d7815d5a
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Fri Sep 17 10:12:12 2021 +0200

    Fix PR rtl-optimization/102306

    This is a duplication of volatile loads introduced during GCC 9 development
    by the 2->2 mechanism of the RTL combiner.  There is already a substantial
    checking for volatile references in can_combine_p but it implicitly assumes
    that the combination reduces the number of instructions, which is of course
    not the case here.  So the fix teaches try_combine to abort the combination
    when it is about to make a copy of volatile references to preserve them.

    gcc/
            PR rtl-optimization/102306
            * combine.c (try_combine): Abort the combination if we are about to
            duplicate volatile references.

    gcc/testsuite/
            * gcc.target/sparc/20210917-1.c: New test.

[Bug rtl-optimization/102306] [9/10/11/12 Regression] Volatile pointer dereferenced twice

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Eric Botcazou
<ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:0c1af84f181237b8a90ae391152dbdcb9c6ab064

commit r10-10134-g0c1af84f181237b8a90ae391152dbdcb9c6ab064
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Fri Sep 17 10:12:12 2021 +0200

    Fix PR rtl-optimization/102306

    This is a duplication of volatile loads introduced during GCC 9 development
    by the 2->2 mechanism of the RTL combiner.  There is already a substantial
    checking for volatile references in can_combine_p but it implicitly assumes
    that the combination reduces the number of instructions, which is of course
    not the case here.  So the fix teaches try_combine to abort the combination
    when it is about to make a copy of volatile references to preserve them.

    gcc/
            PR rtl-optimization/102306
            * combine.c (try_combine): Abort the combination if we are about to
            duplicate volatile references.

    gcc/testsuite/
            * gcc.target/sparc/20210917-1.c: New test.

[Bug rtl-optimization/102306] [9/10/11/12 Regression] Volatile pointer dereferenced twice

[cvs-commit at gcc dot gnu.org]

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

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Eric Botcazou
<ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:57146bfc2459424c007f4b97f29eef4c600a7b8c

commit r9-9732-g57146bfc2459424c007f4b97f29eef4c600a7b8c
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Fri Sep 17 10:12:12 2021 +0200

    Fix PR rtl-optimization/102306

    This is a duplication of volatile loads introduced during GCC 9 development
    by the 2->2 mechanism of the RTL combiner.  There is already a substantial
    checking for volatile references in can_combine_p but it implicitly assumes
    that the combination reduces the number of instructions, which is of course
    not the case here.  So the fix teaches try_combine to abort the combination
    when it is about to make a copy of volatile references to preserve them.

    gcc/
            PR rtl-optimization/102306
            * combine.c (try_combine): Abort the combination if we are about to
            duplicate volatile references.

    gcc/testsuite/
            * gcc.target/sparc/20210917-1.c: New test.

[Bug rtl-optimization/102306] [9/10/11/12 Regression] Volatile pointer dereferenced twice

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #14 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Thanks for reporting the problem.