Re: [JAVA PATCH] SAVE_EXPR evaluate-once semantics in jcf-write.c

[Roger Sayle <roger at eyesopen dot com>]

Hi Per,
> > The following patch implements the required evaluate-once semantics
> > for SAVE_EXPR when generating java bytecodes.  Historically, gcj has
> > just re-evaluated the saved expression which is less than ideal.
>
> My main concern is that localvar_alloc allocates a new local
> variable.  When does it get freed?  There are two concerns:
> (1) It might get freed too early.
> (2) It might never get freed (until the end of the method),
> in which case we may be wasting locals.  This is admittedly
> not a major problem, though it is a bit wasteful.

It never gets freed.  By the time, we're expanding expressions
we don't know the scope of a SAVE_EXPR.  Indeed the scope of
a SAVE_EXPR can easily be the entire method.


> So this ia a work-around, but the real fix is to stop
> generating SAVE_EXPRs while constant-folding.

I (mostly) disagree.  Constant folding rarely generates SAVE_EXPRs
and when it does they tend to be extremely useful, i.e. constant
folding's use of save_expr is appropriate.  The far more common
occurrence of SAVE_EXPR in GCJ, is the Java front-ends misguided
abuse of it to affect evaluation order.

See my recent patch that introduced flag_evaluation_order,
http://gcc.gnu.org/ml/gcc-patches/2003-09/msg01882.html, prior
to which the java front-end was calling save_expr for almost
every binary operator!  It still calls save_expr for most function
arguments!

I agree that maintaining a temporary for the life of a method
is expensive, so SAVE_EXPR needs to be used with care.  Whether
that's by getting rid of unnecessary SAVE_EXPRs or introducing
a new bounded scope SAVE_EXPR, such as the LET_EXPR that you
propose, I don't mind.  However, gcj is clearly not honoring
the required/documented semantics of SAVE_EXPR, and blaming
constant folding for turning "Math.pow(g(x),2.0)" into Java
equivalent "(temp = g(x))*temp" seems unfair.

I'll also admit that we may eventually want to disable constant
folding optimizations that introduced SAVE_EXPRs at some point
in the future, but currently I'd be surprised if these account
for more than 5% of the SAVE_EXPRs encountered by jcf-write.


> What we should do is use a "let expression":
>
> LET tmp := expression IN f(use tmp, use tmp)
>
> This provides an explicit well-defined scope for the temporary.

As mentioned above, this may be a good idea.  I'll let the GIMPLE
folks argue of the merits of allowing a form of SCOPE_EXPR within
expressions.


To conclude, this patch is certainly not a workaround.  Whenever
generate_bytecode_insns encounters a SAVE_EXPR it has to ensure
that its only ever evaluated once.  Those are the semantics of
SAVE_EXPR.  Even if we eventually manage to remove/replace all
SAVE_EXPRs from the tree, the SAVE_EXPR case of this switch either
needs the implementation proposed by my patch or to call abort.
The current scheme of silently generating bad code is "right out".

Roger
--