[C++] PR19531 NRV is performed on volatile temporary

[C++] PR19531 NRV is performed on volatile temporary

[Christian BRUEL]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

Hello,

Here is a fix for PR 19531. testsuite tested for i686-pc-linux-gnu

thanks,

Christian

2007-09-24  Christian Bruel  <christian.bruel@st.com>

	PR c++/19531
	* cp/decl.c (finish_function): Check volatility for nrv.
	
2007-09-24  Christian Bruel  <christian.bruel@st.com>

	PR c++/19531
	* g++.dg/opt/nrv8.C: New.

[-- Attachment #2: vnrv.patch --]
[-- Type: text/plain, Size: 660 bytes --]

Index: gcc/cp/decl.c
===================================================================
--- gcc/cp/decl.c	(revision 128777)
+++ gcc/cp/decl.c	(working copy)
@@ -11729,7 +11729,9 @@
 	     return; see g++.dg/opt/nrv6.C.  We could be more flexible if
 	     we were to do this optimization in tree-ssa.  */
 	  && (outer = outer_curly_brace_block (fndecl))
-	  && chain_member (r, BLOCK_VARS (outer)))
+	  && chain_member (r, BLOCK_VARS (outer))
+	  && !TYPE_VOLATILE (TREE_TYPE (r))
+	  && !TYPE_VOLATILE (TREE_TYPE (TREE_TYPE (fndecl))))
 	finalize_nrv (&DECL_SAVED_TREE (fndecl), r, DECL_RESULT (fndecl));
 
       current_function_return_value = NULL_TREE;

[-- Attachment #3: nrv8.C --]
[-- Type: text/x-c, Size: 409 bytes --]

// PR optimization/19531
// forbids NRV on volatile return value.
// { dg-options "-fno-inline" }
// { dg-do run }

extern "C" { void abort(); }

struct A
{
  int d;

  A ()                     { d = 123; }
  A (const A & o)          { d = o.d;  }
  A (volatile const A & o) { d = o.d + 2; }
};

A bar()
{
  volatile A l;
  return l;
}

main()
{
  A a = bar ();

  if (a.d != 125)
    abort();

  return 0;
}

Re: [C++] PR19531 NRV is performed on volatile temporary

[Mark Mitchell]

Christian BRUEL wrote:

> 2007-09-24  Christian Bruel  <christian.bruel@st.com>
> 
>     PR c++/19531
>     * cp/decl.c (finish_function): Check volatility for nrv.
>     
> 2007-09-24  Christian Bruel  <christian.bruel@st.com>
> 
>     PR c++/19531
>     * g++.dg/opt/nrv8.C: New.

On mainline, even the previous broken check for volatility seems to be
missing.  Does this bug still exist on mainline and does your change
still fix it?

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [C++] PR19531 NRV is performed on volatile temporary

[Christian BRUEL]

Mark Mitchell wrote:
> Christian BRUEL wrote:
> 
> 
>>2007-09-24  Christian Bruel  <christian.bruel@st.com>
>>
>>    PR c++/19531
>>    * cp/decl.c (finish_function): Check volatility for nrv.
>>    
>>2007-09-24  Christian Bruel  <christian.bruel@st.com>
>>
>>    PR c++/19531
>>    * g++.dg/opt/nrv8.C: New.
> 
> 
> On mainline, even the previous broken check for volatility seems to be
> missing.  

In the testsuite there is a 'nrv7.C' and a 'nrv9.C' but no 'nrv8.C'. So 
I picked up this slot, but maybe I should have called the test 'nrv15.C' 
instead ?

Does this bug still exist on mainline and does your change
> still fix it?
> 
> Thanks,
> 

yes it still exists and my change still fixes it.

Regards,

-c

Re: [C++] PR19531 NRV is performed on volatile temporary

[Mark Mitchell]

Christian BRUEL wrote:

>> On mainline, even the previous broken check for volatility seems to be
>> missing.  
> 
> In the testsuite there is a 'nrv7.C' and a 'nrv9.C' but no 'nrv8.C'. So
> I picked up this slot, but maybe I should have called the test 'nrv15.C'
> instead ?

I'm sorry for being unclear.  I wasn't referring to the test number, but
to a line of code in your patch -- which I misread.  So, just ignore
what I said. :-)

> Does this bug still exist on mainline and does your change
>> still fix it?

> yes it still exists and my change still fixes it.

Is the check for volatility on both the return type of the function --
TREE_TYPE (TREE_TYPE (fndecl)) -- and on the RESULT_DECL -- TREE_TYPE
(r)) -- necessary?

I would expect that checking the volatility of the function return type
is sufficient; that's the language issue.  If, for whatever reason, the
compiler were to make the RESULT_DECL volatile, for a function with a
non-volatile return, I believe that the NRV would still be valid.

Would you please test removing the check on the RESULT_DECL from your patch?

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [C++] PR19531 NRV is performed on volatile temporary

[Christian BRUEL]

Mark Mitchell wrote:
<< snap>>

> Is the check for volatility on both the return type of the function --
> TREE_TYPE (TREE_TYPE (fndecl)) -- and on the RESULT_DECL -- TREE_TYPE
> (r)) -- necessary?
> 
> I would expect that checking the volatility of the function return type
> is sufficient; that's the language issue.  If, for whatever reason, the
> compiler were to make the RESULT_DECL volatile, for a function with a
> non-volatile return, I believe that the NRV would still be valid.
> 
> Would you please test removing the check on the RESULT_DECL from your patch?
> 

it is the other way: with the nrv8.C (attached in the patch) test, only 
the check on the RESULT_DECL is necessary, testing the volatility of the 
*volatile A l;* object.  Without it the check doesn't pass.
I think this is what 12.8 15 was originally about, before Core DR20 
extended it to the function's return type.

I also though that there would be a language issue with the volatility 
of the function's return type, but the volatility flag is not set on the 
return type of the cpctor. Should it be ? I'm not sure since with:
A (volatile const A & o) { d = o.d + 2; }
the cpctor return type is not volatile, only the temporary object is, 
but I'd prefer to let that to the experts's hands.

So I conservatively added the check on the function's return type to 
match core dr20 extension (quoted in the bugzilla entry but I don't have 
it) although I was not able to make up a test case.

Best Regards
Christian

Re: [C++] PR19531 NRV is performed on volatile temporary

[Mark Mitchell]

Christian BRUEL wrote:

> it is the other way: with the nrv8.C (attached in the patch) test, only
> the check on the RESULT_DECL is necessary, testing the volatility of the
> *volatile A l;* object.

Oh!  Right, top-level qualifiers on return types are discarded from the
point of view of the language.

But, sadly, I'm still confused.  Your example is:

A bar()
{
  volatile A l;
  return l;
}

Why is RESULT_DECL volatile in this case, before we call finalize_nrv?
Is this is just an accident because we have only one return statement?
For example:

A bar(bool b) {
  A a;
  volatile A va;
  if (b)
    return a;
  return va;
}

Is the RESULT_DECL still volatile?

It seems like it would be simpler to fix this problem by changing
check_return_expr to set named_return_value_okay_p to false if the
TREE_TYPE (retval) is volatile.  What do you think about that?

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [C++] PR19531 NRV is performed on volatile temporary

[Christian BRUEL]

Mark Mitchell wrote:
> Christian BRUEL wrote:
> 
> 
>>it is the other way: with the nrv8.C (attached in the patch) test, only
>>the check on the RESULT_DECL is necessary, testing the volatility of the
>>*volatile A l;* object.
> 
> 
> Oh!  Right, top-level qualifiers on return types are discarded from the
> point of view of the language.
> 
> But, sadly, I'm still confused.  Your example is:
> 
> A bar()
> {
>   volatile A l;
>   return l;
> }
> 
> Why is RESULT_DECL volatile in this case, before we call finalize_nrv?
> Is this is just an accident because we have only one return statement?
> For example:
> 
> A bar(bool b) {
>   A a;
>   volatile A va;
>   if (b)
>     return a;
>   return va;
> }
> 
> Is the RESULT_DECL still volatile?
> 

In this example there are 2 retvals with 2 TREE_TYPEs, so one 
TREE_TYPE's is volatile and the other is not. But here NRV is avoided 
because there are 2 different return values, regardless of one that is 
volatile.

> It seems like it would be simpler to fix this problem by changing
> check_return_expr to set named_return_value_okay_p to false if the
> TREE_TYPE (retval) is volatile.  What do you think about that?
> 

yes you are right, it's would be a better place to do the check. but...

In fact the code is already there :

      /* The cv-unqualified type of the returned value must be the
         same as the cv-unqualified return type of the
         function.  */
      && same_type_p ((TYPE_MAIN_VARIANT (TREE_TYPE (retval))),
                      (TYPE_MAIN_VARIANT
                       (TREE_TYPE (TREE_TYPE(current_function_decl))))));

Indeed, I reread the parts on volatile and object copying, the standard 
states nrv is allowed if the cv-unqualified types match, so strictly 
speaking
the object
  volatile A a;
and the function
  A bar()
have the same cv-unqualified type...

so this volatile optimization is surprisingly correct !

Nevertheless, it would have been useful to have a way to force a call to 
the copy constructor...  but the standard allows this optimisation here, 
that's it.

Unless there is a consensus that this optimisation could be avoided on 
volatile objects for convenience, this is not a bug and I will update 
PR19531 accordingly.

Best Regards

Christian

Re: [C++] PR19531 NRV is performed on volatile temporary

[Christian BRUEL]

sorry I was looking at ISO/IEC 14882 (first edition). In 14882:2003 this 
is indeed restricted to non-volatile automatic object. please ignore 
this comment. thx

Christian

> Indeed, I reread the parts on volatile and object copying, the standard 
> states nrv is allowed if the cv-unqualified types match, so strictly 
> speaking
> the object
>  volatile A a;
> and the function
>  A bar()
> have the same cv-unqualified type...
> 
> so this volatile optimization is surprisingly correct !
> 
> Nevertheless, it would have been useful to have a way to force a call to 
> the copy constructor...  but the standard allows this optimisation here, 
> that's it.
> 
> Unless there is a consensus that this optimisation could be avoided on 
> volatile objects for convenience, this is not a bug and I will update 
> PR19531 accordingly.
> 
> Best Regards
> 
> Christian
> 
> 
> 
> 
> 
> 
> 
> 

Re: [C++] PR19531 NRV is performed on volatile temporary

[Christian BRUEL]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

please find attached the revisited fix.

thanks,

-c

2007-09-24 Christian Bruel <christian.bruel@st.com>
            Mark Mitchell <mark@codesourcery.com>

         PR c++/19531
         * cp/typeck.c (check_return_expr): Don't set 
named_return_value_okay_p if retval is volatile.
	
2007-09-24  Christian Bruel  <christian.bruel@st.com>

         PR c++/19531
         * g++.dg/opt/nrv8.C: New.

Mark Mitchell wrote:
> Christian BRUEL wrote:
> 
> 
>>it is the other way: with the nrv8.C (attached in the patch) test, only
>>the check on the RESULT_DECL is necessary, testing the volatility of the
>>*volatile A l;* object.
> 
> 
> Oh!  Right, top-level qualifiers on return types are discarded from the
> point of view of the language.
> 
> But, sadly, I'm still confused.  Your example is:
> 
> A bar()
> {
>   volatile A l;
>   return l;
> }
> 
> Why is RESULT_DECL volatile in this case, before we call finalize_nrv?
> Is this is just an accident because we have only one return statement?
> For example:
> 
> A bar(bool b) {
>   A a;
>   volatile A va;
>   if (b)
>     return a;
>   return va;
> }
> 
> Is the RESULT_DECL still volatile?
> 
> It seems like it would be simpler to fix this problem by changing
> check_return_expr to set named_return_value_okay_p to false if the
> TREE_TYPE (retval) is volatile.  What do you think about that?
> 


[-- Attachment #2: vnrv.patch --]
[-- Type: text/plain, Size: 635 bytes --]

Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 129768)
+++ gcc/cp/typeck.c	(working copy)
@@ -6744,7 +6744,9 @@
         function.  */
      && same_type_p ((TYPE_MAIN_VARIANT (TREE_TYPE (retval))),
                      (TYPE_MAIN_VARIANT
-                      (TREE_TYPE (TREE_TYPE (current_function_decl))))));
+                      (TREE_TYPE (TREE_TYPE (current_function_decl)))))
+     /* And the returned value must be non-volatile.  */
+     && !TYPE_VOLATILE (TREE_TYPE (retval)));
      
   if (fn_returns_value_p && flag_elide_constructors)
     {

[-- Attachment #3: nrv8.C --]
[-- Type: text/x-c, Size: 399 bytes --]

// PR optimization/19531
// forbids NRV on volatile return value.
// { dg-options -O2 }
// { dg-do run }

extern "C" { void abort(); }

struct A
{
  int d;

  A ()                     { d = 123; }
  A (const A & o)          { d = o.d;  }
  A (volatile const A & o) { d = o.d + 2; }
};

A bar()
{
  volatile A l;
  return l;
}

main()
{
  A a = bar ();

  if (a.d != 125)
    abort();

  return 0;
}

Re: [C++] PR19531 NRV is performed on volatile temporary

[Mark Mitchell]

Christian BRUEL wrote:

> 2007-09-24 Christian Bruel <christian.bruel@st.com>
>            Mark Mitchell <mark@codesourcery.com>
> 
>         PR c++/19531
>         * cp/typeck.c (check_return_expr): Don't set
> named_return_value_okay_p if retval is volatile.
>     
> 2007-09-24  Christian Bruel  <christian.bruel@st.com>
> 
>         PR c++/19531
>         * g++.dg/opt/nrv8.C: New.

This patch is OK, thanks.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713