[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472 --- Comment #14 from Andrey Belevantsev 2011-01-13 10:04:18 UTC --- Do we want at least the patch properly merging the volatile bits in the scheduler for 4.6? Or is this better be s plain ICE instead of a silent miscompile?
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472 Richard Guenther changed: What|Removed |Added Target Milestone|4.5.2 |4.5.3 --- Comment #13 from Richard Guenther 2010-12-16 13:03:14 UTC --- GCC 4.5.2 is being released, adjusting target milestone.
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
--- Comment #12 from Paolo Bonzini 2010-10-18 17:12:59
UTC ---
It would be nice if for
struct a {
char a,b,c,d;
volatile int e;
};
struct a v1, v2;
...
v1 = v2;
the compiler emitted only _two_ memory accesses, one for a/b/c/d and one for e.
I'm not sure a MEM_REF(&vv1) = MEM_REF(&vv2) with volatile arguments would
achieve this.
So, even though it's just an optimization, it would be better if the IR was
designed to allow it. Would this be the case if the front-end emitted a
volatile MODIFY_EXPR?
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
--- Comment #11 from joseph at codesourcery dot com 2010-10-18 16:41:03 UTC ---
On Mon, 18 Oct 2010, rguenth at gcc dot gnu.org wrote:
> Also consider memcpy (&vv1, &vv2) and eventually the compiler optimizing
> that to vv1 = vv2 (note the lack of {v} here).
Using memcpy when any part of the source or destination is an object
defined with volatile type has undefined behavior (at runtime).
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472 --- Comment #10 from Michael Matz 2010-10-18 15:58:26 UTC --- One idea we had was that this is all frontends business anyway, and hence it should (if it so desires) simply create volatile MEM_REFs for references to half-volatile objects. That alone would result in the copy statement being marked volatile, and would also (I guess, haven't checked) do the right thing in expand. So, if we (the frontend) decide that accesses to objects containing volatile subobjects should itself be regarded as volatile, then generating the right kind of MEM_REF would already provide that.
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
--- Comment #9 from Richard Guenther 2010-10-18
15:42:44 UTC ---
(In reply to comment #8)
> Would it make sense to make the statement volatile even if only some
> subcomponents (or all subcomponents) are volatile?
>
> I like (2); if I understand it correctly, in this case vv1 and vv2 would not
> be
> volatile, but you'd still have
>
>vv1 ={v} vv2;
>
> in the GIMPLE source. It should be possible to use a bit on
> {ARRAY,RECORD,UNION,QUAL_UNION}_TYPE to cache this, e.g.
>
> #define TYPE_HAS_VOLATILE_PARTS(T) \
>(AGGREGATE_TYPE_P (T) \
> ? TYPE_UNSIGNED (T) \
> : TYPE_VOLATILE (T))
>
> #define AGGREGATE_TYPE_CHECK(T) \
>TREE_CHECK4(T, ARRAY_TYPE, RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE)
>
> #define SET_TYPE_HAS_VOLATILE_PARTS(T, V) \
>(TYPE_UNSIGNED (AGGREGATE_TYPE_CHECK (T)) = (V))
>
> Separately, expand would of course need to be taught about expanding accesses
> to volatile subcomponents as mem/v. If this approach was feasible, it would
> have the advantage of splitting the task in two parts, one for GIMPLE
> (including possibly the verifier) and one for expand.
If we want to treat vv1 = vv2 as volatile then the frontends can now simply
emit MEM_REF <&vv1> = MEM_REF <&vv2> with TREE_THIS_VOLATILE set and things
should work. That leaves it up to the frontend on how to deal with this.
The much harder question is how to expand vv1 = vv2 "correctly". Thus,
we need to define what happens and document it.
Also consider memcpy (&vv1, &vv2) and eventually the compiler optimizing
that to vv1 = vv2 (note the lack of {v} here).
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
--- Comment #8 from Paolo Bonzini 2010-10-18 12:20:39
UTC ---
Would it make sense to make the statement volatile even if only some
subcomponents (or all subcomponents) are volatile?
I like (2); if I understand it correctly, in this case vv1 and vv2 would not be
volatile, but you'd still have
vv1 ={v} vv2;
in the GIMPLE source. It should be possible to use a bit on
{ARRAY,RECORD,UNION,QUAL_UNION}_TYPE to cache this, e.g.
#define TYPE_HAS_VOLATILE_PARTS(T) \
(AGGREGATE_TYPE_P (T) \
? TYPE_UNSIGNED (T) \
: TYPE_VOLATILE (T))
#define AGGREGATE_TYPE_CHECK(T) \
TREE_CHECK4(T, ARRAY_TYPE, RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE)
#define SET_TYPE_HAS_VOLATILE_PARTS(T, V) \
(TYPE_UNSIGNED (AGGREGATE_TYPE_CHECK (T)) = (V))
Separately, expand would of course need to be taught about expanding accesses
to volatile subcomponents as mem/v. If this approach was feasible, it would
have the advantage of splitting the task in two parts, one for GIMPLE
(including possibly the verifier) and one for expand.
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
Michael Matz changed:
What|Removed |Added
CC||jason at gcc dot gnu.org,
||jsm28 at gcc dot gnu.org
--- Comment #7 from Michael Matz 2010-10-18 11:46:57
UTC ---
Well, the problem is not only in expand. As you say we start with this
code:
...
vv1 = vv2;
...
That is already a non-volatile statement on the gimple level. Making the
whole vv1/vv2 object volatile at the top-level of course changes this
into a volatile statement:
...
vv1 ={v} vv2;
...
The problem with this is that at the gimple level the volatility is not
represented, and hence could already result in invalid transformations.
This extends into expand, so in a way the wrong MEM expressions are
a result of that.
Assuming we'd extend expand to expand such block moves, where parts of the
block are volatile into a series of volatile accesses, we could fix the bug
at hand. That would still leave the problem of non-volatility on the gimple
level.
This all points to a deeper problem IMO, one we need to decide how to solve
before tackling the details of this bug, namely: how are non-volatile objects
containing volatile subobjects handled? I don't know if the language
standard says anything about this.
We have several possibilities:
1) make all objects containing volatile subobjects volatile itself
2) make instructions touching such objects volatile
3) make instructions touching the volatile components volatile
The first has the problem that volatile objects with aggregate type
implicitely contain only volatile subobjects, that is for such object:
struct {int a; volatile int b;} half_volatile;
with solution (1) this would be equivalent to
volatile struct {int a; volatile int b;} half_volatile;
and hence halt_volatile.a would be volatile too, probably unlike the author
intended.
The other two solutions are more work, especially because the half-volatility
wouldn't be reflected in the objects type.
Independend of what we decide for the middle-end, I'd like to hear the opinion
of the C frontend people about this issue.
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472 --- Comment #6 from Andrey Belevantsev 2010-10-18 10:57:23 UTC --- Anybody familiar with the expand/tree level can take a look on this? I can spare some time later this week if folks are busy.
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
--- Comment #5 from bonzini at gnu dot org 2010-09-20 16:01 --- Looks like a problem in expand. CCing Matz. -- bonzini at gnu dot org changed: What|Removed |Added CC||matz at gcc dot gnu dot org http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
--- Comment #4 from amonakov at gcc dot gnu dot org 2010-09-20 14:49
---
A small testcase to illustrate the problem with volatile fields.
//---8<---
struct vv {volatile long a, b;} vv1, vv2;
int foo()
{
vv1 = vv2;
}
//---8<---
gcc/cc1 -O2 -frename-registers -fschedule-insns2 vol.c
movqvv2+8(%rip), %rax
movqvv2(%rip), %rdx
movq%rax, vv1+8(%rip)
movq%rdx, vv1(%rip)
The compiler reorders accesses to volatile fields. As Andrey said, /v bits are
missing on MEMs even in the .expand dump.
--
amonakov at gcc dot gnu dot org changed:
What|Removed |Added
CC||amonakov at gcc dot gnu dot
||org
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
--- Comment #3 from abel at gcc dot gnu dot org 2010-09-20 13:05 ---
We have the code like this:
if (...)
{
17 cx:DI=[`s2'] //comes from s2.vl += s1.vl;
...
}
27 dx:DI=[`s2'] //comes from s1 = s2;
When the scheduler tries to move insn 27 before if (...), it also unifies its
right-hand sides, as they seem equal. The scheduler wants to get:
27 dx:DI=[`s2']
if (...)
{
cx = dx
...
dx:DI=[`s2'] // bookkeeping
}
The insn 17 has its MEM with volatile bit set, the insn 27 has it unset. It so
happens that when gathering the available insn set and when moving the actually
selected insn, insns 17 and 27 got merged in the different order. First we
don't have the volatile bit on the resulting insn, thus we believe the load
does not trap and we move it up through a jump before the 'if'. Second we have
the bit and thus insn traps and we don't move it, then we hit the consistency
assert in the scheduler.
Now, I'm happy to implement the correct merging of the may_trap_p bit in the
scheduler which would "fix" this. However, looking at the original C code it
looks like both MEMs should have their volatile bit set. I can only say that
the original bits seem to come from expand, the addresses got propagated by
fwprop but this doesn't seem to be the issue.
--
abel at gcc dot gnu dot org changed:
What|Removed |Added
CC||abel at gcc dot gnu dot org
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
-- rguenth at gcc dot gnu dot org changed: What|Removed |Added Priority|P3 |P2 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
-- rguenth at gcc dot gnu dot org changed: What|Removed |Added Target Milestone|--- |4.5.2 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
--- Comment #2 from hjl dot tools at gmail dot com 2010-09-01 03:03 --- It is caused by revision 147282: http://gcc.gnu.org/ml/gcc-cvs/2009-05/msg00256.html -- hjl dot tools at gmail dot com changed: What|Removed |Added CC||bonzini at gnu dot org Status|UNCONFIRMED |NEW Ever Confirmed|0 |1 Last reconfirmed|-00-00 00:00:00 |2010-09-01 03:03:31 date|| http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2
--- Comment #1 from zsojka at seznam dot cz 2010-08-31 23:58 --- Created an attachment (id=21630) --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21630&action=view) reduced testcase $ gcc -O1 -fschedule-insns2 -fselective-scheduling2 pr45472.c -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472
