Bug 16115, C++ invisible references (was: Reverting your patch from2004-06-24)

[Jason Merrill <jason at redhat dot com>]

On Tue, 6 Jul 2004 00:23:43 -0700, Mark Mitchell <mark@codesourcery.com> wrote:

> Jason --
>
> I've reverted this patch:
>
> 	2004-06-24  Jason Merrill  <jason@redhat.com>
> 	PR c++/16115
> 	* decl.c (grokparms): Give the PARM_DECL reference type if the
> 	parameter is passed by invisible reference.
>
> because (a) it's slightly buggy, and (b) I think it's the wrong
> approach.
>
> I recognize that I've exceeded my mandate here, in that I've just
> reverted the patch without the usual 48-hour deal, but I'm counting on
> the fact that you and I have always resolved differences effectively
> to allow me to avoid massive wrath from the direction of greater
> Boston.  If you're not happy, well, revert my reversion, and we'll
> find another way to work it out.

Reverting the patch before discussion strikes me as an odd overreaction,
but I feel no wrath about it.  :)

I've been continuing to work in this area; I'll just wait and check in the
improved version later.

> This patch is incorrect, on several levels.
>
> First, it changes the type of the PARM_DECL to be different from the
> type that C++ assigns that declaration.  That is incorrect; the C++
> front end's assignment of types should match the language.  As we've
> discussed previously, for example, it's a bug that we have expressions
> with REFERENCE_TYPE; the standard says that no expressions have that
> type.  (Declarations can, of course, have REFERENCE_TYPE -- but these
> parameters do not.)  We should be trying to minimize the extent to
> which ABI details appear in the front end.

I disagree with this.  It seems to me that you're striving towards a goal
of turning the IL into a mirror of the source, which is nice to keep as an
ideal, but impractical in many cases.  'for' scoping comes to mind; the
scope of a declaration in the for-init-statement is larger than its
sub-tree, so to do anything sensible with it we need to move it outside the
loop construct.  Within the compiler, we need to represent things in an
internally consistent way, which may not always match the convoluted C++
syntax.  This last came up in the context of the patch to use
STATEMENT_LISTs for C and C++, and I suppose it's an issue that needs to be
resolved separately: How hard should we try to retain C++ syntactic
structure during parsing?  My own feeling is: as much as possible when
parsing templates, as much as convenient otherwise.

In this case, the parameter is in fact passed as a reference.  The C++
language requires that the object live past the end of the function for the
lifetime of temporaries rules to work properly.  Representing it as a local
object is lying to the compiler, and Bug 16115 is an example of it having
its revenge.  Representing parms and returns as references when they are
passed as such also allows us to clean up a lot of special cases in the
inliner.

I don't think we have expressions with REFERENCE_TYPE; all occurrences of a
reference immediately decay.  I would prefer a design that didn't require
us to scatter convert_from_reference calls everywhere, but I agree with the
<INDIRECT_REF <decl/call>> tree structure that results.

ABI details pervade the front end.  I don't see how it could be otherwise.
Besides, while passing these types by invisible reference happens to be
described by the C++ ABI, I can't think of any other implementation that
both conforms to the standard and allows elision of temporaries.

> Furthermore, this test case:
>
>   template <typename T>
>   struct S {
>     ~S();
>   };
>   struct T {
>     T(S<int>);
>     S<int> s_;
>   };
>   T::T(S<int> s) : s_(s) {
>   }
>
> now ICEs the compiler on (at least) powerpc-apple-darwin7.4.0 due to
> some problem with cloned functions.

I'll take a look.

> Also, this change is potentially pessimizing code in that it will no
> longer be obvious to the optimizers that the static type of the
> parameter must in fact be the declared type.  So, we may not be able
> to resolve virtual function invoked on the parameter.

True.  rth also pointed out that we should set __restrict on the reference,
since nothing else can refer to these stack slots; the issue you mention
calls for a GENERIC-level type assertion just like we want for
new-expressions.

> Finally, if we were going to make this change in the front end, the
> right place would be in grokdeclarator (where the PARM_DECL is
> created) rather than in grokparms, where we must then wastefully
> create a second PARM_DECL.

Yes, that was sloppy; my current code avoids creating a second PARM_DECL.

> If the optimizers needs these parameters to have reference type, then
> that change needs to happen after the front end processing is
> complete, in the course of generating code for this function.  I'm not
> sure what the tradeoffs are vis a vis (re)teaching the optimizers
> about TREE_ADDRESSABLE on types.

You mean teaching the optimizers that a PARM_DECL of TREE_ADDRESSABLE type
lives beyond the end of the function?  That sounds like rather complex
semantics to expect them to hold on to.  I much prefer to make as much as
possible explicit in the GENERIC.

Your other suggestion gets back to the question above of conformity to the
source.  Is it worth the cost of building and throwing away all these extra
nodes just to have the parse trees match the C++ syntax more closely?

> (Parenthetically, I'm not sure the call1.C test is checking anything
> useful, since it does not fail even after I reverted the patch.)

Probably because I forgot to set dg-options to -O2.  Corrected.

Jason