Re: [PATCH] tree-optimization/110434 - avoid <retval> ={v} {CLOBBER} from NRV

Re: [PATCH] tree-optimization/110434 - avoid <retval> ={v} {CLOBBER} from NRV

[Jakub Jelinek]

On Wed, Jun 28, 2023 at 10:21:45AM +0000, Richard Biener via Gcc-patches wrote:
> When NRV replaces a local variable with <retval> it also replaces
> occurences in clobbers.  This leads to <retval> being clobbered
> before the return of it which is strictly invalid but harmless in
> practice since there's no pass after NRV which would remove
> earlier stores.
> 
> The following fixes this nevertheless.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
> 
> Thanks,
> Richard.
> 
> 	PR tree-optimization/110434
> 	* tree-nrv.cc (pass_nrv::execute): Remove CLOBBERs of
> 	VAR we replace with <retval>.

This is in a loop over all basic blocks in a function.
Do we want to kill all clobbers, or just the ones at the end of functions
(i.e. after the <result> = VAR; assignment that we also remove)?
Complication is that doesn't necessarily have to be just the rest of
a single basic block, but all basic blocks from that point until end of
function.
I mean, if we have
  var = whatever;
  use (var);
  var = {CLOBBER};
  ...
  var = whatever_else;
  <result> = var;
  var = {CLOBBER};
killing the first clobber might result in missed optimizations later on.

On the other side, could there be partial clobbers for the var -> <result>,
  var.fld = {CLOBBER};
?  Or even worse, indirect clobbers (MEM_REF with SSA_NAME pointing to
var or parts of it)?

	Jakub

Re: [PATCH] tree-optimization/110434 - avoid <retval> ={v} {CLOBBER} from NRV

[Richard Biener]

On Wed, 28 Jun 2023, Jakub Jelinek wrote:

> On Wed, Jun 28, 2023 at 10:21:45AM +0000, Richard Biener via Gcc-patches wrote:
> > When NRV replaces a local variable with <retval> it also replaces
> > occurences in clobbers.  This leads to <retval> being clobbered
> > before the return of it which is strictly invalid but harmless in
> > practice since there's no pass after NRV which would remove
> > earlier stores.
> > 
> > The following fixes this nevertheless.
> > 
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
> > 
> > Thanks,
> > Richard.
> > 
> > 	PR tree-optimization/110434
> > 	* tree-nrv.cc (pass_nrv::execute): Remove CLOBBERs of
> > 	VAR we replace with <retval>.
> 
> This is in a loop over all basic blocks in a function.
> Do we want to kill all clobbers, or just the ones at the end of functions
> (i.e. after the <result> = VAR; assignment that we also remove)?
> Complication is that doesn't necessarily have to be just the rest of
> a single basic block, but all basic blocks from that point until end of
> function.
> I mean, if we have
>   var = whatever;
>   use (var);
>   var = {CLOBBER};
>   ...
>   var = whatever_else;
>   <result> = var;
>   var = {CLOBBER};
> killing the first clobber might result in missed optimizations later on.

As said there's nothing run after NRV.

> 
> On the other side, could there be partial clobbers for the var -> <result>,
>   var.fld = {CLOBBER};
> ?  Or even worse, indirect clobbers (MEM_REF with SSA_NAME pointing to
> var or parts of it)?

We know that 'var' is not address taken, not sure about the partial
clobbers.  We could deal with this in the walk_gimple_op case and
simply remove a clobber when data.modified.

I went at it under the presumption that <retval> never goes out of
scope so we shouldn't have any CLOBBER for it.  You could also say
that NRV should operate flow-sensitive, going from returns backwards
and simply stop replacing when the var it substitutes for is clobbered.

I'll do the adjustment handling var.fld = {CLOBBER};

If we don't remove earlier clobbers shouldn't that prevent the NRV
then?

Richard.

Re: [PATCH] tree-optimization/110434 - avoid <retval> ={v} {CLOBBER} from NRV

[Jakub Jelinek]

On Wed, Jun 28, 2023 at 12:32:51PM +0000, Richard Biener wrote:
> As said there's nothing run after NRV.

There is expansion but in the <result> case I strongly doubt we are trying
to stack reuse it for other vars, so maybe it is ok.

> > On the other side, could there be partial clobbers for the var -> <result>,
> >   var.fld = {CLOBBER};
> > ?  Or even worse, indirect clobbers (MEM_REF with SSA_NAME pointing to
> > var or parts of it)?
> 
> We know that 'var' is not address taken, not sure about the partial
> clobbers.  We could deal with this in the walk_gimple_op case and
> simply remove a clobber when data.modified.

LGTM.

	Jakub

Re: [PATCH] tree-optimization/110434 - avoid <retval> ={v} {CLOBBER} from NRV

[Jeff Law]

On 6/28/23 04:21, Richard Biener via Gcc-patches wrote:
> When NRV replaces a local variable with <retval> it also replaces
> occurences in clobbers.  This leads to <retval> being clobbered
> before the return of it which is strictly invalid but harmless in
> practice since there's no pass after NRV which would remove
> earlier stores.
> 
> The following fixes this nevertheless.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
> 
> Thanks,
> Richard.
> 
> 	PR tree-optimization/110434
> 	* tree-nrv.cc (pass_nrv::execute): Remove CLOBBERs of
> 	VAR we replace with <retval>.
OK.
jeff

[PATCH] tree-optimization/110434 - avoid <retval> ={v} {CLOBBER} from NRV

[Richard Biener]

When NRV replaces a local variable with <retval> it also replaces
occurences in clobbers.  This leads to <retval> being clobbered
before the return of it which is strictly invalid but harmless in
practice since there's no pass after NRV which would remove
earlier stores.

The following fixes this nevertheless.

Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?

Thanks,
Richard.

	PR tree-optimization/110434
	* tree-nrv.cc (pass_nrv::execute): Remove CLOBBERs of
	VAR we replace with <retval>.
---
 gcc/tree-nrv.cc | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gcc/tree-nrv.cc b/gcc/tree-nrv.cc
index ff47439647c..466b491e4e7 100644
--- a/gcc/tree-nrv.cc
+++ b/gcc/tree-nrv.cc
@@ -256,6 +256,14 @@ pass_nrv::execute (function *fun)
 	      gsi_remove (&gsi, true);
 	      release_defs (stmt);
 	    }
+	  /* If this is a CLOBBER of VAR, remove it.  */
+	  else if (gimple_clobber_p (stmt)
+		   && gimple_assign_lhs (stmt) == found)
+	    {
+	      unlink_stmt_vdef (stmt);
+	      gsi_remove (&gsi, true);
+	      release_defs (stmt);
+	    }
 	  else
 	    {
 	      struct walk_stmt_info wi;
-- 
2.35.3