|Summary:||[Regression]: volatileness of write is discarded, perhaps in "lim1" related to loop optimizations|
|Product:||gcc||Reporter:||Hans-Peter Nilsson <hp>|
|Component:||middle-end||Assignee:||Not yet assigned to anyone <unassigned>|
|Build:||Known to work:|
|Known to fail:||Last reconfirmed:|
Preprocessed code; compile at -O2, e.g. "cc1 -O2 y.i -o y.s"
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