[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[Bug rtl-optimization/45472] [4.5/4.6 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[abel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #6 from Andrey Belevantsev <abel at gcc dot gnu.org> 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

[matz at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

Michael Matz <matz at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org,
                   |                            |jsm28 at gcc dot gnu.org

--- Comment #7 from Michael Matz <matz at gcc dot gnu.org> 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

[bonzini at gnu dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #8 from Paolo Bonzini <bonzini at gnu dot org> 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

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #9 from Richard Guenther <rguenth at gcc dot gnu.org> 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

[matz at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #10 from Michael Matz <matz at gcc dot gnu.org> 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

[joseph at codesourcery dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #11 from joseph at codesourcery dot com <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

[bonzini at gnu dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #12 from Paolo Bonzini <bonzini at gnu dot org> 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

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.5.2                       |4.5.3

--- Comment #13 from Richard Guenther <rguenth at gcc dot gnu.org> 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

[abel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #14 from Andrey Belevantsev <abel at gcc dot gnu.org> 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 middle-end/45472] [4.5/4.6/4.7 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[abel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #15 from Andrey Belevantsev <abel at gcc dot gnu.org> 2011-04-08 06:42:22 UTC ---
Just to remind the middle-end folks that stage 1 is a fine time for deciding on
middle-end volatile semantics, before we forget about this for another 6 months
:)

[Bug middle-end/45472] [4.5/4.6/4.7 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.5.3                       |4.5.4

--- Comment #16 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-04-28 14:51:25 UTC ---
GCC 4.5.3 is being released, adjusting target milestone.

[Bug middle-end/45472] [4.5/4.6/4.7 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|2010-09-01 03:03:31         |2011-12-09

--- Comment #17 from Andrew Pinski <pinskia at gcc dot gnu.org> 2011-12-10 03:57:18 UTC ---
By the way I think we could get cases where the user wrote volatile in one case
and non-volatile in another so fixing up the merging is still a good idea.

[Bug middle-end/45472] [4.5/4.6/4.7 Regression] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[abel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #18 from Andrey Belevantsev <abel at gcc dot gnu.org> 2012-01-19 09:28:56 UTC ---
(In reply to comment #17)
> By the way I think we could get cases where the user wrote volatile in one case
> and non-volatile in another so fixing up the merging is still a good idea.

Sure, but the comment 14 still applies -- I think that unless the middle-end
will have the proper volatile semantics, it will be wrong to hide this issue in
the scheduler, so I'm leaving this as it is for now.

[Bug middle-end/45472] [4.5/4.6/4.7 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[jason at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #19 from Jason Merrill <jason at gcc dot gnu.org> 2012-02-16 19:41:29 UTC ---
It seems to me that volatile reads/writes should get their own gimple
statements, not be part of a larger block move.  So instead of

  vv1 = vv2;

we should have

  vv1.a ={v} vv2.a;
  vv1.b ={v} vv2.b;

I agree with Paolo's comment in #12 that we want to copy the non-volatile parts
as a block when possible.  It seems like breaking a simple struct assignment
into these separate statements would be best done in the gimplifier so that
front ends don't need to get this right independently.

Out of curiousity, what is the use case for a non-volatile struct object with
volatile members?

[Bug middle-end/45472] [4.5/4.6/4.7 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[zsojka at seznam dot cz]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #20 from Zdenek Sojka <zsojka at seznam dot cz> 2012-02-16 20:14:54 UTC ---
I can think of two use-cases from threaded environment:
- using the volatile member as a semaphore for the structure
- when one needs to assure some data will be written in certain order (eg.
first write data, then the 'data valid' flag), while other members of the
structure don't need to be volatile (data used by only one thread, or data that
are only read; marking the whole struct volatile would prevent some
optimisations)

Other cases:
- when debugging and you want to prevent optimisations of certain variable
- when the structure is allocated on a memory-mapped IO address, and only some
parts of that IO need to be marked as volatile
- in OOP, where the variable is a member, and would otherwise be a global
volatile variable

[Bug middle-end/45472] [4.5/4.6/4.7 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #21 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-20 11:43:01 UTC ---
(In reply to comment #19)
> It seems to me that volatile reads/writes should get their own gimple
> statements, not be part of a larger block move.  So instead of
> 
>   vv1 = vv2;
> 
> we should have
> 
>   vv1.a ={v} vv2.a;
>   vv1.b ={v} vv2.b;
> 
> I agree with Paolo's comment in #12 that we want to copy the non-volatile parts
> as a block when possible.  It seems like breaking a simple struct assignment
> into these separate statements would be best done in the gimplifier so that
> front ends don't need to get this right independently.
> 
> Out of curiousity, what is the use case for a non-volatile struct object with
> volatile members?

There is no valid use-case for this.  So I think we should just declare this
issue a non-issue (middle-end wise).  If the C or C++ standards say that
vv1 = vv2 should behave as if the copy was elementwise then the frontends
need changing.  Certainly not the gimplifier - that's not the kitchen-sink
for things you don't want to properly describe in GENERIC to the middle-end ;)

[Bug middle-end/45472] [4.5/4.6/4.7 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #22 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-27 08:45:52 UTC ---
> If the C or C++ standards say that vv1 = vv2 should behave as if the copy was
> elementwise then the frontends need changing.  Certainly not the gimplifier -
> that's not the kitchen-sink for things you don't want to properly describe in 
> GENERIC to the middle-end ;)

Yes, I think that we wouldn't want this in Ada for example.

[Bug middle-end/45472] [4.5/4.6/4.7/4.8 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.5.4                       |4.6.4

--- Comment #23 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-07-02 11:19:03 UTC ---
The 4.5 branch is being closed, adjusting target milestone.

[Bug middle-end/45472] [4.6/4.7/4.8 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[dje at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

David Edelsohn <dje at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|x86_64-pc-linux-gnu         |x86_64-pc-linux-gnu,
                   |                            |powerpc*-*-*
                 CC|                            |dje at gcc dot gnu.org

--- Comment #24 from David Edelsohn <dje at gcc dot gnu.org> 2013-01-05 21:12:05 UTC ---
This ICE has started to appear for target POWER when compiling
testsuite/gcc.dg/tree-prof/pr50907.c

$ /tmp/20130104/gcc/xgcc -B/tmp/20130104/gcc/
/nasfarm/dje/src/src/gcc/testsuite/gcc.dg/tree-prof/pr50907.c 
-fno-diagnostics-show-caret   -O -freorder-blocks-and-partition
-fschedule-insns -fselective-scheduling -fpic -pthread -fprofile-generate
-D_PROFILE_GENERATE  -lm   -o /tmp/20130104/gcc/testsuite/gcc/pr50907.x01

/nasfarm/dje/src/src/gcc/testsuite/gcc.dg/tree-prof/pr45354.c: In function
'test_ifelse2':
/nasfarm/dje/src/src/gcc/testsuite/gcc.dg/tree-prof/pr45354.c:23:1: internal
compiler error: in move_op_ascend, at sel-sched.c:6153

[Bug middle-end/45472] [4.6/4.7/4.8 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[zsojka at seznam dot cz]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #25 from Zdenek Sojka <zsojka at seznam dot cz> 2013-01-05 21:43:55 UTC ---
(In reply to comment #24)
> This ICE has started to appear for target POWER when compiling
> testsuite/gcc.dg/tree-prof/pr50907.c
> 

That testcase (or pr45354.c which it includes) doesn't contain any 'volatile'
keyword, so it is likely a different bug. I would recommend you opening a
separate PR for that. This PR is about treating 'volatile' on struct members.

[Bug middle-end/45472] [4.6/4.7/4.8 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[abel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #26 from Andrey Belevantsev <abel at gcc dot gnu.org> 2013-02-26 09:21:14 UTC ---
Created attachment 29539
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29539
scheduler patch

I'm testing the attached patch to fix the ICE in the scheduler.  Then the
issue/non-issue of the front-end of not presenting the proper volatile bits to
the middle-end can be discussed separately.

[Bug middle-end/45472] [4.6/4.7/4.8 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[abel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #27 from Andrey Belevantsev <abel at gcc dot gnu.org> 2013-02-27 08:56:15 UTC ---
Author: abel
Date: Wed Feb 27 08:56:08 2013
New Revision: 196308

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=196308
Log:
        PR middle-end/45472

    gcc/
        * sel-sched-ir.c (merge_expr): Also change vinsn of merged expr
        when the may_trap_p bit of the exprs being merged differs.

        Reorder tests for speculativeness in the logical and operator.

    testsuite/
    * gcc.dg/45472.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr45472.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/sel-sched-ir.c
    trunk/gcc/testsuite/ChangeLog

[Bug middle-end/45472] [4.6/4.7 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[abel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

Andrey Belevantsev <abel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |4.8.0
            Summary|[4.6/4.7/4.8 Regression]    |[4.6/4.7 Regression]
                   |[Middle-end volatile        |[Middle-end volatile
                   |semantics] ICE: in          |semantics] ICE: in
                   |move_op_ascend, at          |move_op_ascend, at
                   |sel-sched.c:6124 with       |sel-sched.c:6124 with
                   |-fselective-scheduling2     |-fselective-scheduling2

--- Comment #28 from Andrey Belevantsev <abel at gcc dot gnu.org> 2013-02-27 09:04:31 UTC ---
Fixed on trunk.

[Bug middle-end/45472] [4.6/4.7 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[abel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

--- Comment #29 from Andrey Belevantsev <abel at gcc dot gnu.org> 2013-04-03 05:59:53 UTC ---
Ported to 4.7 and 4.6, though no bugzilla commit made.  Do I close the bug or
do we want to fix the front-end to produce the proper volatile bits?

[Bug middle-end/45472] [4.7 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45472

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.6.4                       |4.7.4

--- Comment #30 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-04-12 15:15:58 UTC ---
GCC 4.6.4 has been released and the branch has been closed.

[Bug middle-end/45472] [4.7 Regression] [Middle-end volatile semantics] ICE: in move_op_ascend, at sel-sched.c:6124 with -fselective-scheduling2

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|4.7.4                       |4.8.0
      Known to fail|                            |4.7.4

--- Comment #31 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed for 4.8.0