[Bug d/110516] New: core.volatile.volatileLoad is broken

[Bug d/110516] New: core.volatile.volatileLoad is broken

[witold.baryluk+gcc at gmail dot com]

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

            Bug ID: 110516
           Summary: core.volatile.volatileLoad is broken
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: d
          Assignee: ibuclaw at gdcproject dot org
          Reporter: witold.baryluk+gcc at gmail dot com
  Target Milestone: ---

gcc 12.2.0 (from Debian stable) and gcc trunk 14.0.0 (in godbolt) tested.

core.volatile.volatileLoad simply does not work.

1) It merges loads.
2) It removes unused loads at -O1 and higher.

Example:

void actualRun(ubyte* ptr1) {
  import core.volatile : volatileLoad;
  volatileLoad(ptr1);
  volatileLoad(ptr1);
  volatileLoad(ptr1);
  volatileLoad(ptr1);
}


Without optimisations:

void example.actualRun(ubyte*):
        push    rbp
        mov     rbp, rsp
        mov     QWORD PTR [rbp-8], rdi
        nop
        pop     rbp
        ret


Incorrect.



With optimisations:

void example.actualRun(ubyte*):
        ret

Incorrect.


Expected:

void example.actualRun(ubyte*):
        movzx   eax, byte ptr [rdi]
        movzx   eax, byte ptr [rdi]
        movzx   eax, byte ptr [rdi]
        movzx   eax, byte ptr [rdi]
        ret



dmd and ldc behave properly.


It looks like it never worked properly.

Would be good to have a test case for this, so it does not become a regression
later.


I did not test volatileStore, but I would not be surprised it is also broken.

[Bug d/110516] core.volatile.volatileLoad is broken

[ibuclaw at gcc dot gnu.org]

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

ibuclaw at gcc dot gnu.org changed:

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

--- Comment #1 from ibuclaw at gcc dot gnu.org ---
(In reply to Witold Baryluk from comment #0)
> gcc 12.2.0 (from Debian stable) and gcc trunk 14.0.0 (in godbolt) tested.
> 
> core.volatile.volatileLoad simply does not work.
> 
> 1) It merges loads.
> 2) It removes unused loads at -O1 and higher.
> 
> Example:
> 
> void actualRun(ubyte* ptr1) {
>   import core.volatile : volatileLoad;
>   volatileLoad(ptr1);
>   volatileLoad(ptr1);
>   volatileLoad(ptr1);
>   volatileLoad(ptr1);
> }
Actually, doesn't seem to be doing either 1 or 2, it looks like the statement
is just dropped altogether as no obvious side-effect was picked up from the
expression to which the intrinsic was expanded into.

https://d.godbolt.org/z/361x4vKfn

[Bug d/110516] core.volatile.volatileLoad is broken

[ibuclaw at gcc dot gnu.org]

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

--- Comment #2 from ibuclaw at gcc dot gnu.org ---
(In reply to Witold Baryluk from comment #0)
> I did not test volatileStore, but I would not be surprised it is also broken.
volatileStore is fine, because that expands to an assignment, which is assumed
to have a side effect.

[Bug d/110516] core.volatile.volatileLoad discarded if result is unused

[cvs-commit at gcc dot gnu.org]

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

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain Buclaw <ibuclaw@gcc.gnu.org>:

https://gcc.gnu.org/g:80ae426a195a0d035640a6301da833564deade52

commit r14-2236-g80ae426a195a0d035640a6301da833564deade52
Author: Iain Buclaw <ibuclaw@gdcproject.org>
Date:   Sun Jul 2 03:24:53 2023 +0200

    d: Fix core.volatile.volatileLoad discarded if result is unused

    The first pass of code generation in the D front-end splits up all
    compound expressions and discards expressions that have no side effects.
    This included calls to the `volatileLoad' intrinsic if its result was
    not used, causing such calls to be eliminated from the program.

    We already set TREE_THIS_VOLATILE on the expression, however the
    tree documentation says if this bit is set in an expression, so is
    TREE_SIDE_EFFECTS.  So set TREE_SIDE_EFFECTS on the expression too.
    This prevents any early discarding from occuring.

            PR d/110516

    gcc/d/ChangeLog:

            * intrinsics.cc (expand_volatile_load): Set TREE_SIDE_EFFECTS on
the
            expanded expression.
            (expand_volatile_store): Likewise.

    gcc/testsuite/ChangeLog:

            * gdc.dg/torture/pr110516a.d: New test.
            * gdc.dg/torture/pr110516b.d: New test.

[Bug d/110516] core.volatile.volatileLoad discarded if result is unused

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Iain Buclaw
<ibuclaw@gcc.gnu.org>:

https://gcc.gnu.org/g:78e9bd776d559b4a2193d61dd074ee0880d43ec5

commit r13-7521-g78e9bd776d559b4a2193d61dd074ee0880d43ec5
Author: Iain Buclaw <ibuclaw@gdcproject.org>
Date:   Sun Jul 2 03:24:53 2023 +0200

    d: Fix core.volatile.volatileLoad discarded if result is unused

    The first pass of code generation in the D front-end splits up all
    compound expressions and discards expressions that have no side effects.
    This included calls to the `volatileLoad' intrinsic if its result was
    not used, causing such calls to be eliminated from the program.

    We already set TREE_THIS_VOLATILE on the expression, however the
    tree documentation says if this bit is set in an expression, so is
    TREE_SIDE_EFFECTS.  So set TREE_SIDE_EFFECTS on the expression too.
    This prevents any early discarding from occuring.

            PR d/110516

    gcc/d/ChangeLog:

            * intrinsics.cc (expand_volatile_load): Set TREE_SIDE_EFFECTS on
the
            expanded expression.
            (expand_volatile_store): Likewise.

    gcc/testsuite/ChangeLog:

            * gdc.dg/torture/pr110516a.d: New test.
            * gdc.dg/torture/pr110516b.d: New test.

    (cherry picked from commit 80ae426a195a0d035640a6301da833564deade52)

[Bug d/110516] core.volatile.volatileLoad discarded if result is unused

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Iain Buclaw
<ibuclaw@gcc.gnu.org>:

https://gcc.gnu.org/g:b224692084e40aabcba1e73166eedf55dfd7f563

commit r12-9749-gb224692084e40aabcba1e73166eedf55dfd7f563
Author: Iain Buclaw <ibuclaw@gdcproject.org>
Date:   Sun Jul 2 03:24:53 2023 +0200

    d: Fix core.volatile.volatileLoad discarded if result is unused

    The first pass of code generation in the D front-end splits up all
    compound expressions and discards expressions that have no side effects.
    This included calls to the `volatileLoad' intrinsic if its result was
    not used, causing such calls to be eliminated from the program.

    We already set TREE_THIS_VOLATILE on the expression, however the
    tree documentation says if this bit is set in an expression, so is
    TREE_SIDE_EFFECTS.  So set TREE_SIDE_EFFECTS on the expression too.
    This prevents any early discarding from occuring.

            PR d/110516

    gcc/d/ChangeLog:

            * intrinsics.cc (expand_volatile_load): Set TREE_SIDE_EFFECTS on
the
            expanded expression.
            (expand_volatile_store): Likewise.

    gcc/testsuite/ChangeLog:

            * gdc.dg/torture/pr110516a.d: New test.
            * gdc.dg/torture/pr110516b.d: New test.

    (cherry picked from commit 80ae426a195a0d035640a6301da833564deade52)

[Bug d/110516] core.volatile.volatileLoad discarded if result is unused

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Iain Buclaw
<ibuclaw@gcc.gnu.org>:

https://gcc.gnu.org/g:da7a10ea9245d7461253495698873891ffc20666

commit r11-10890-gda7a10ea9245d7461253495698873891ffc20666
Author: Iain Buclaw <ibuclaw@gdcproject.org>
Date:   Sun Jul 2 03:24:53 2023 +0200

    d: Fix core.volatile.volatileLoad discarded if result is unused

    The first pass of code generation in the D front-end splits up all
    compound expressions and discards expressions that have no side effects.
    This included calls to the `volatileLoad' intrinsic if its result was
    not used, causing such calls to be eliminated from the program.

    We already set TREE_THIS_VOLATILE on the expression, however the
    tree documentation says if this bit is set in an expression, so is
    TREE_SIDE_EFFECTS.  So set TREE_SIDE_EFFECTS on the expression too.
    This prevents any early discarding from occuring.

            PR d/110516

    gcc/d/ChangeLog:

            * intrinsics.cc (expand_volatile_load): Set TREE_SIDE_EFFECTS on
the
            expanded expression.
            (expand_volatile_store): Likewise.

    gcc/testsuite/ChangeLog:

            * gdc.dg/torture/pr110516a.d: New test.
            * gdc.dg/torture/pr110516b.d: New test.

    (cherry picked from commit 80ae426a195a0d035640a6301da833564deade52)

[Bug d/110516] core.volatile.volatileLoad discarded if result is unused

[ibuclaw at gcc dot gnu.org]

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

ibuclaw at gcc dot gnu.org changed:

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

--- Comment #7 from ibuclaw at gcc dot gnu.org ---
Fix committed and backported.

[Bug d/110516] core.volatile.volatileLoad discarded if result is unused

[witold.baryluk+gcc at gmail dot com]

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

--- Comment #8 from Witold Baryluk <witold.baryluk+gcc at gmail dot com> ---
I see.

Point 1 is definitively incorrect. I interpreted asembler wrong:

void example.actualRun(ubyte*):
        push    rbp
        mov     rbp, rsp
        mov     QWORD PTR [rbp-8], rdi
        nop
        pop     rbp
        ret


The move there, is just some stack manipulation, it has nothing to do with
volatileLoad.



You are right about the side effect visibility and volatileStore.

Still, there should be a way to express real memory read, with result not
stored anywhere in program (just written to register, then discarded).

This has some (not very common) uses in memmory-mapped IO, i.e. in drivers for
devices where the read itself could indicate something (this of course usually
also require setting proper page table attributes to disable caching or other
optimizations, etc, not just volatile load in machine code). I do not have
specific examples at hand, but afaik I saw some examples in the past (mostly on
older architectures), as well some watchdog chips that reset timer on read.

Another use is for doing memory and cache read benchmarks and profiling. We
want to invoke read (to register) from some memory location, but we do not need
the value for anything else.

And more esoteric use might be memory probing. On some level systems, kernel or
bootloader, might not know the memory layout, and resort to just doing reads,
and relaying on CPU fault handlers to report invalid reads.

And some people might use load without destination, as a prefetch hint, or to
prefault some memory pages.

[Bug d/110516] core.volatile.volatileLoad discarded if result is unused

[witold.baryluk+gcc at gmail dot com]

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

--- Comment #9 from Witold Baryluk <witold.baryluk+gcc at gmail dot com> ---
Thank you for a quick fix Iain!