Bug 58145 - [Regression]: volatileness of write is discarded, perhaps in "lim1" related to loop optimizations
Summary: [Regression]: volatileness of write is discarded, perhaps in "lim1" related t...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.9.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2013-08-13 02:38 UTC by Hans-Peter Nilsson
Modified: 2013-08-16 01:32 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
Preprocessed code; compile at -O2, e.g. "cc1 -O2 y.i -o y.s" (240 bytes, text/plain)
2013-08-13 02:38 UTC, Hans-Peter Nilsson
Details
untested fix (561 bytes, patch)
2013-08-13 22:56 UTC, Jakub Jelinek
Details | Diff
gcc49-pr58145.patch (1.06 KB, patch)
2013-08-14 10:09 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans-Peter Nilsson 2013-08-13 02:38:33 UTC
Created attachment 30640 [details]
Preprocessed code; compile at -O2, e.g. "cc1 -O2 y.i -o y.s"

The exact version in which the bug appeared is not yet triaged: it's present on r201675 of trunk, r201652 of the 4.8 branch, r190527 of the 4.7 branch (!) but appears to not be present in r135713 of the 4.3 branch (!).

The bug is that the volatileness of the dereference of the write (the assignment through a pointer to a volatile structure) in function pb_out is discarded, leaving a single write after the loop.  Note also that together with the discarded-volatileness-bug there seems to be a missed-optimization-bug in that the loop is redundant; the loop awkwardly computes iterates over 0..31 and computes 1<<i but the intermediate computations aren't used; then the last value is written after the loop. Editing the code to manually inline pb_out makes no difference to the bug.

The wrong code is evident already in the .expand dump on trunk (according to -da).  It is not present (according to -fdump-tree-all-all) in y.i.096t.loopinit but appears present in y.i.097t.lim1.

Until someone (including myself) has repeated the observation for another target, I'll set the target-specifier to cris*-* but it seems obviously generic, affecting all targets.
Comment 1 Jakub Jelinek 2013-08-13 22:56:20 UTC
Created attachment 30648 [details]
untested fix

Seems to be a tree-sra.c bug to me.  Untested fix attached, with no testcase yet etc.
Comment 2 Jakub Jelinek 2013-08-14 10:09:21 UTC
Created attachment 30653 [details]
gcc49-pr58145.patch

Updated patch.
Comment 3 Hans-Peter Nilsson 2013-08-14 15:18:07 UTC
(In reply to Jakub Jelinek from comment #2)
> Created attachment 30653 [details]
> gcc49-pr58145.patch
> 
> Updated patch.

Thank you very much, Jakub!
The missing opportunity to learn trees :) is offset by far by the value of the promptness of the patch and the right generic incantations for the test-case!

I'll test and regtest this, though the exact change in volatileness won't be tested beyond the test-case and the code-base where this was spotted.  Still, apparently the patch can only add volatileness indicators where none was before, so should be safe even for other branches than trunk.

It's obvious to you and other tree-ssa-savvy people, but IMHO its notable that fiddling with the test-case reveals that the bug seems limited to singleton-bit-field-structures of "natural" sizes; 8, 16, 32, (64 etc. where applicable).  E.g. changing the bit-field-size to 9 or having two bitfields of 16 bits does not trig the bug.
Comment 4 Jakub Jelinek 2013-08-14 20:37:13 UTC
Author: jakub
Date: Wed Aug 14 20:34:56 2013
New Revision: 201748

URL: http://gcc.gnu.org/viewcvs?rev=201748&root=gcc&view=rev
Log:
	PR tree-optimization/58145
	* tree-sra.c (build_ref_for_offset): If prev_base has
	TREE_THIS_VOLATILE or TREE_SIDE_EFFECTS, propagate it to MEM_REF.

	* gcc.dg/pr58145-1.c: New test.
	* gcc.dg/pr58145-2.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr58145-1.c
    trunk/gcc/testsuite/gcc.dg/pr58145-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-sra.c

Author: jakub
Date: Wed Aug 14 20:36:12 2013
New Revision: 201749

URL: http://gcc.gnu.org/viewcvs?rev=201749&root=gcc&view=rev
Log:
	PR tree-optimization/58145
	* tree-sra.c (build_ref_for_offset): If prev_base has
	TREE_THIS_VOLATILE or TREE_SIDE_EFFECTS, propagate it to MEM_REF.

	* gcc.dg/pr58145-1.c: New test.
	* gcc.dg/pr58145-2.c: New test.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/pr58145-1.c
    branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/pr58145-2.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_8-branch/gcc/tree-sra.c
Comment 5 Hans-Peter Nilsson 2013-08-16 01:32:39 UTC
Formally repeated (using the F17 cut of 4.7.2 (gcc-4.7.2-2.fc17.x86_64) and trunk of r200585 on x86_64-linux) as not target-specific.