preexpand_calls vs. TARGET_EXPRs

[Mark Mitchell <mark at codesourcery dot com>]

Please read this mail if you are someone who knows about
preexpand_calls.  Even though some of this message is C++-specific,
there are generic questions here, too.

preexpand_calls is used to pre-evaluate all the CALL_EXPRs in an
expression tree.  The results of the calls are stored in the
CALL_EXPR_RTL for the CALL_EXPR.  Later, when the expression tree is
evaluated for real, the CALL_EXPRs simply expand to the corresponding
CALL_EXPR_RTL.

Mostly, we do this whenever we hit a binary operation in tree, so
that (for example), when evaluating:

  (f () + 1) + (g (), 8)

we pre-evaluate the calls to `f' and `g' before expanding the
PLUS_EXPR.

I'm not sure exactly what the point of that it is.  It doesn't seem to
be for correctness (unless there's something about the
ever-troublesome do_pending_stack_adjust -- and in that case there's
probably still a bug since preexpand_calls doesn't expand builtins,
even though they can sometimes result in actualy function calls).
Therefore, I assume it's some attempt at optimization.  (If so, it's
probably misguided; we should be depending on real algorithms to
reorder code, not playing games during tree-expansion.)

In any case, preexpanding calls is not correct in C++.

Here's a concrete example:

  PLUS_EXPR
    ADDR_EXPR
      TARGET_EXPR
	CALL_EXPR
    INTEGER_CST

Assume that the innermost CALL_EXPR returns a structure, by value.
Assume further that the structure type has a "copy constructor".  In
other words, the language specifies that this type cannot be copied
bitwise; it must always be copied by calling the magic copy
constructor function.  (Furthermore, when and how many times that
function is called is specified by the language.)

Now, the whole point of a TARGET_EXPR is that it contains a target; it
wants the CALL_EXPR expanded into a particular spot, given by the
TARGET_EXPR.  Unfortunately, the PLUS_EXPR results in preexpand_calls
getting called first, and that function doesn't know to pass down the
right target.  Therefore, the CALL puts its structure result in the
wrong place, and a copy is introduced later when the TARGET_EXPR is
expanded for real.

I think there is another problem too -- expressions that introduce a
new binding level (like STMT_EXPR) are traversed by preexpand calls,
meaning that temporaries are probably placed in a sub-optimal stack
level.  That could result in cleanups getting run in the wrong order,
but I don't see anything that suggests this is too likely.

There are various ways that this could be fixed:

  - Turn off do_preexpand_calls in C++.

  - Make preexpand_calls smarter to know what it can safely expand
    and what it can't.
 
  - Remove preexpand_calls.

I favor the last option.  This stuff is ugly as heck.  Removing
preexpand_calls would also allow us to remove CALL_EXPR_RTL, which is
a headache since it introduces a nasty statefulness into trees that
makes them harder to copy/share.  (We should strive to have the only
state introduced by RTL-expansion be DECL_RTL -- and maybe even elide
that at some point.)

Comments?

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com