public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug c++/110754] New: assume create spurious load for volatile variable
@ 2023-07-20 19:16 muecker at gwdg dot de
2023-07-20 22:34 ` [Bug c++/110754] " xry111 at gcc dot gnu.org
` (10 more replies)
0 siblings, 11 replies; 12+ messages in thread
From: muecker at gwdg dot de @ 2023-07-20 19:16 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
Bug ID: 110754
Summary: assume create spurious load for volatile variable
Product: gcc
Version: 14.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: c++
Assignee: unassigned at gcc dot gnu.org
Reporter: muecker at gwdg dot de
Target Milestone: ---
With optimization, using assume with volatile create spurious loads:
int bar(int p)
{
volatile int n = p;
[[assume (1 == n)]];
return 1 + n;
}
bar(int):
mov DWORD PTR [rsp-4], edi
mov eax, DWORD PTR [rsp-4]
mov eax, DWORD PTR [rsp-4]
add eax, 1
ret
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug c++/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
@ 2023-07-20 22:34 ` xry111 at gcc dot gnu.org
2023-07-20 22:42 ` [Bug middle-end/110754] " pinskia at gcc dot gnu.org
` (9 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: xry111 at gcc dot gnu.org @ 2023-07-20 22:34 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
Xi Ruoyao <xry111 at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |xry111 at gcc dot gnu.org
--- Comment #1 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
It happens w/o assume:
int bar(int p)
{
volatile int n = p;
if (1 == n)
__builtin_unreachable();
return 1 + n;
}
Is this a bug? The standard defines accessing volatile objects as side-effects
so it's not allowed to merge volatile loads, AFAIU.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug middle-end/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
2023-07-20 22:34 ` [Bug c++/110754] " xry111 at gcc dot gnu.org
@ 2023-07-20 22:42 ` pinskia at gcc dot gnu.org
2023-07-20 22:44 ` pinskia at gcc dot gnu.org
` (8 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-20 22:42 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
Andrew Pinski <pinskia at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Last reconfirmed| |2023-07-20
Status|UNCONFIRMED |NEW
Ever confirmed|0 |1
Keywords| |wrong-code
Component|c++ |middle-end
--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Xi Ruoyao from comment #1)
> Is this a bug? The standard defines accessing volatile objects as
> side-effects so it's not allowed to merge volatile loads, AFAIU.
Yes because assume attribute is defined not to have any side effects.
Confirmed.
gimplifier produces:
[[assume (D.2786)]]
{
{
int n.0;
n.0 = n;
D.2786 = n.0 == 1;
}
}
And then lowering produces:
_2 = n;
.ASSUME (_Z3bari._assume.0, _2);
But really it should have passed the address of n rather than the value since n
is volatile here .
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug middle-end/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
2023-07-20 22:34 ` [Bug c++/110754] " xry111 at gcc dot gnu.org
2023-07-20 22:42 ` [Bug middle-end/110754] " pinskia at gcc dot gnu.org
@ 2023-07-20 22:44 ` pinskia at gcc dot gnu.org
2023-07-20 22:45 ` xry111 at gcc dot gnu.org
` (7 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-20 22:44 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Seems like lowering passes everything via value rather than some stuff by
reference ....
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug middle-end/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
` (2 preceding siblings ...)
2023-07-20 22:44 ` pinskia at gcc dot gnu.org
@ 2023-07-20 22:45 ` xry111 at gcc dot gnu.org
2023-07-21 6:44 ` rguenth at gcc dot gnu.org
` (6 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: xry111 at gcc dot gnu.org @ 2023-07-20 22:45 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
--- Comment #4 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #2)
> (In reply to Xi Ruoyao from comment #1)
> > Is this a bug? The standard defines accessing volatile objects as
> > side-effects so it's not allowed to merge volatile loads, AFAIU.
>
> Yes because assume attribute is defined not to have any side effects.
>
> Confirmed.
>
> gimplifier produces:
>
> [[assume (D.2786)]]
> {
> {
> int n.0;
>
> n.0 = n;
> D.2786 = n.0 == 1;
> }
> }
>
> And then lowering produces:
> _2 = n;
> .ASSUME (_Z3bari._assume.0, _2);
>
> But really it should have passed the address of n rather than the value
> since n is volatile here .
Alright, I mistakenly believed [[assume(x)]]; is same as if (!x)
unreachable();.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug middle-end/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
` (3 preceding siblings ...)
2023-07-20 22:45 ` xry111 at gcc dot gnu.org
@ 2023-07-21 6:44 ` rguenth at gcc dot gnu.org
2023-07-21 8:33 ` muecker at gwdg dot de
` (5 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: rguenth at gcc dot gnu.org @ 2023-07-21 6:44 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
An assumption like this is quite useless anyway, but ...
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug middle-end/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
` (4 preceding siblings ...)
2023-07-21 6:44 ` rguenth at gcc dot gnu.org
@ 2023-07-21 8:33 ` muecker at gwdg dot de
2024-02-09 9:15 ` jakub at gcc dot gnu.org
` (4 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: muecker at gwdg dot de @ 2023-07-21 8:33 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
--- Comment #6 from Martin Uecker <muecker at gwdg dot de> ---
One should check whether there is a similar issue with atomics, at least
regarding accidentally introducing memory ordering or so.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug middle-end/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
` (5 preceding siblings ...)
2023-07-21 8:33 ` muecker at gwdg dot de
@ 2024-02-09 9:15 ` jakub at gcc dot gnu.org
2024-02-09 15:00 ` jakub at gcc dot gnu.org
` (3 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-02-09 9:15 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For volatile I'm afraid we can't pass n as an argument to the magic function,
perhaps
we could pass its address to it and do the load in there. Perhaps ditto for
the atomics (that is C only of course), but that likely is what happens
already.
Sure, most likely such assumptions won't be really useful for anything, but
one could still say
volatile int n = p;
int m = 0;
[[assume (p == (n & m))]];
if (p != 0)
...
or similar.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug middle-end/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
` (6 preceding siblings ...)
2024-02-09 9:15 ` jakub at gcc dot gnu.org
@ 2024-02-09 15:00 ` jakub at gcc dot gnu.org
2024-02-10 10:28 ` cvs-commit at gcc dot gnu.org
` (2 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-02-09 15:00 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Assignee|unassigned at gcc dot gnu.org |jakub at gcc dot gnu.org
Status|NEW |ASSIGNED
--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 57373
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57373&action=edit
gcc14-pr110754.patch
Untested fix.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug middle-end/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
` (7 preceding siblings ...)
2024-02-09 15:00 ` jakub at gcc dot gnu.org
@ 2024-02-10 10:28 ` cvs-commit at gcc dot gnu.org
2024-03-02 0:38 ` cvs-commit at gcc dot gnu.org
2024-03-04 12:09 ` jakub at gcc dot gnu.org
10 siblings, 0 replies; 12+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2024-02-10 10:28 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
--- Comment #9 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:
https://gcc.gnu.org/g:39920447f876128ff7942a9cd931021800865894
commit r14-8910-g39920447f876128ff7942a9cd931021800865894
Author: Jakub Jelinek <jakub@redhat.com>
Date: Sat Feb 10 11:28:00 2024 +0100
gimple-low: Fix up handling of volatile automatic vars in assume attribute
[PR110754]
As the following testcases show, the gimple-low outlining of assume
magic functions handled volatile automatic vars (including
parameters/results) like non-volatile ones except it copied volatile
to the new PARM_DECL, which has the undesirable effect that a load
from the volatile var is passed to IFN_ASSUME and so there is a
side-effect there even when side-effects of the assume attribute
shouldn't be evaluated.
The following patch fixes that by passing address of the volatile
variables/parameters/results instead and doing loads or stores from it
or to it where it was originally accessed in the assume attribute
expression.
2024-02-10 Jakub Jelinek <jakub@redhat.com>
PR middle-end/110754
* gimple-low.cc (assumption_copy_decl): For TREE_THIS_VOLATILE
decls create PARM_DECL with pointer to original type, set
TREE_READONLY and keep TREE_THIS_VOLATILE, TREE_ADDRESSABLE,
DECL_NOT_GIMPLE_REG_P and DECL_BY_REFERENCE cleared.
(adjust_assumption_stmt_op): For remapped TREE_THIS_VOLATILE decls
wrap PARM_DECL into a simple TREE_THIS_NO_TRAP MEM_REF.
(lower_assumption): For TREE_THIS_VOLATILE vars pass ADDR_EXPR
of the var as argument.
* gcc.dg/attr-assume-6.c: New test.
* g++.dg/cpp23/attr-assume12.C: New test.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug middle-end/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
` (8 preceding siblings ...)
2024-02-10 10:28 ` cvs-commit at gcc dot gnu.org
@ 2024-03-02 0:38 ` cvs-commit at gcc dot gnu.org
2024-03-04 12:09 ` jakub at gcc dot gnu.org
10 siblings, 0 replies; 12+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2024-03-02 0:38 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
--- Comment #10 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:
https://gcc.gnu.org/g:e084a6406ea0587beda62684b9d4856292acacfa
commit r13-8389-ge084a6406ea0587beda62684b9d4856292acacfa
Author: Jakub Jelinek <jakub@redhat.com>
Date: Sat Feb 10 11:28:00 2024 +0100
gimple-low: Fix up handling of volatile automatic vars in assume attribute
[PR110754]
As the following testcases show, the gimple-low outlining of assume
magic functions handled volatile automatic vars (including
parameters/results) like non-volatile ones except it copied volatile
to the new PARM_DECL, which has the undesirable effect that a load
from the volatile var is passed to IFN_ASSUME and so there is a
side-effect there even when side-effects of the assume attribute
shouldn't be evaluated.
The following patch fixes that by passing address of the volatile
variables/parameters/results instead and doing loads or stores from it
or to it where it was originally accessed in the assume attribute
expression.
2024-02-10 Jakub Jelinek <jakub@redhat.com>
PR middle-end/110754
* gimple-low.cc (assumption_copy_decl): For TREE_THIS_VOLATILE
decls create PARM_DECL with pointer to original type, set
TREE_READONLY and keep TREE_THIS_VOLATILE, TREE_ADDRESSABLE,
DECL_NOT_GIMPLE_REG_P and DECL_BY_REFERENCE cleared.
(adjust_assumption_stmt_op): For remapped TREE_THIS_VOLATILE decls
wrap PARM_DECL into a simple TREE_THIS_NO_TRAP MEM_REF.
(lower_assumption): For TREE_THIS_VOLATILE vars pass ADDR_EXPR
of the var as argument.
* gcc.dg/attr-assume-6.c: New test.
* g++.dg/cpp23/attr-assume12.C: New test.
(cherry picked from commit 39920447f876128ff7942a9cd931021800865894)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug middle-end/110754] assume create spurious load for volatile variable
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
` (9 preceding siblings ...)
2024-03-02 0:38 ` cvs-commit at gcc dot gnu.org
@ 2024-03-04 12:09 ` jakub at gcc dot gnu.org
10 siblings, 0 replies; 12+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-03-04 12:09 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110754
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
Status|ASSIGNED |RESOLVED
Target Milestone|--- |13.3
--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Should be now fixed for GCC 13.3+ too.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-03-04 12:09 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-20 19:16 [Bug c++/110754] New: assume create spurious load for volatile variable muecker at gwdg dot de
2023-07-20 22:34 ` [Bug c++/110754] " xry111 at gcc dot gnu.org
2023-07-20 22:42 ` [Bug middle-end/110754] " pinskia at gcc dot gnu.org
2023-07-20 22:44 ` pinskia at gcc dot gnu.org
2023-07-20 22:45 ` xry111 at gcc dot gnu.org
2023-07-21 6:44 ` rguenth at gcc dot gnu.org
2023-07-21 8:33 ` muecker at gwdg dot de
2024-02-09 9:15 ` jakub at gcc dot gnu.org
2024-02-09 15:00 ` jakub at gcc dot gnu.org
2024-02-10 10:28 ` cvs-commit at gcc dot gnu.org
2024-03-02 0:38 ` cvs-commit at gcc dot gnu.org
2024-03-04 12:09 ` jakub at gcc dot gnu.org
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).