stabilize_reference question

[Mike Stump]

> Date: Mon, 08 Mar 1999 18:12:38 -0800
> From: Per Bothner <bothner@cygnus.com>

> Can somebody explain the comment in the code below?

Sure, I can give it a try, I wrote it.  :-)

> The first expression is not supposed to be "ignored" if volatile;

The documentation seems to be at odds with you?:

/* Contains two expressions to compute, one followed by the other.
   the first value is ignored.  The second one's value is used.  The
   type of the first expression need not agree with the other types.  */
DEFTREECODE (COMPOUND_EXPR, "compound_expr", 'e', 2)

> it is supposed to be expanded once if it appears once in the source.
> At least, that is my understanding.

> In other words:  What is wrong about this patch?

g++.old-deja/g++.mike/p9506.C might be a testcase, check PR 9506 to see
if it was done around:

from /gd2/cc/tree.c:
revision 1.136
date: 1996/05/06 18:07:22;  author: mrs;  state: Exp;  lines: +4 -4
        * expr.c (expand_increment): Add third parameter to know when to
        ignore the result value.
        (store_constructor): Ditto.
        (expand_expr, case PREINCREMENT_EXPR): Ditto.
        (expand_expr, case PREDECREMENT_EXPR): Ditto.
        (expand_expr, case POSTINCREMENT_EXPR): Ditto.
        (expand_expr, case POSTDECREMENT_EXPR): Ditto.
        * tree.c (stabilize_reference): Always ignore the first operand of
        COMPOUND_EXPRs.

Mon May  6 11:12:39 1996  Mike Stump  <mrs@cygnus.com>
 
        * expr.c (expand_increment): Add third parameter to know when to
        ignore the result value.
        (store_constructor): Likewise
        (expand_expr, case {PRE,POST}{INC,DEC}REMENT_EXPR): Likewise.
        * tree.c (stabilize_reference): Always ignore the first operand of
        COMPOUND_EXPRs.

Though, as I recall I thought it was a net reported bug.  I went
through 100% of the semantics of volatile, and fixed every single last
bug.  volatile isn't something that should have _any_ bugs in it.

> (In Java, things woudl be a little simpler and cleaner
> if we could make this fix.)

I will guarantee it will break the semantics of volatile in C and/or
C++.  I don't have an easy access to gcc2 archives from May 6th, 1996
or so, check them and you will find my testcase and the verbose
description.  Hum, maybe a pointer from the egcs website to an ftpable
gcc2 archives might be good.

> +      result = build_nt (COMPOUND_EXPR,
> +			 save_expr (TREE_OPERAND (ref, 0)),
> +			 stabilize_reference (TREE_OPERAND (ref, 1)));

Gee, this is just as you wrote it originally.  The code should look
familiar to you!


The short answer is that stabilizing references have semantics, and
those semantics cannot change due to semantic preserving
optimizations.  stabilization is supposed to be value preserving,
right?  The extra save_expr causes the recursive expand_expr to be
called with a real live target, not with the value that it had (would
have had), which is (from expand_expr):

    case COMPOUND_EXPR:
      expand_expr (TREE_OPERAND (exp, 0), const0_rtx, VOIDmode, 0);
      emit_queue ();
      return expand_expr (TREE_OPERAND (exp, 1),
			   (ignore ? const0_rtx : target),
				       VOIDmode, 0);

const0_rtx.  You propose to change the target from const0_rtx, to a
real target, this isn't appropriate.  The target of the first expr
when expanded _must_ always be const0_rtx, no matter what.  Now, why
does this matter?  I think the testcase in C was:

	volatile int i;
 
	main() { ++i; }

and the problem was that we'd get a spurious read from i.  Now, do we
_have_ to use COMPOUND_EXPRs like this?  No, but we would need a
replacement for them and all languages converted to use that
replacement that care (C and/or C++ at a minimum as I recall).  If you
want to invent a `this value is ignored and should only be expanded
once' tree code, go ahead, and you can use that tree code.  Be sure
to read up on UNSAVE_EXPR, if you do.

Ah, hum...  Possibly we can alter SAVE_EXPR?.  I checked SAVE_EXPR
semantics, and it has a VOID_MODE provision no-a-days:

      if (mode == VOIDmode)
          temp = const0_rtx;

Maybe that wasn't in there back then?  If you can trace the code
around and ensure that const0_rtx is used if you use that, we should
be ok.  Please dig out my testcase and ensure that the bug I fixed,
stays fixed.

Let me know how is goes.  I am interested.