Re: PR80806

[Richard Biener <rguenther@suse.de>]

On Thu, 29 Jun 2017, Jeff Law wrote:

> On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote:
> > Hi,
> > The attached patch tries to fix PR80806 by warning when a variable is
> > set using memset (and friends) but not used. I chose to warn in dse
> > pass since dse would detect if the variable passed as 1st argument is
> > a dead store. Does this approach look OK ?
> [ ... ]
> So I think the biggest question is whether or not the case like Martin's
> deserves a warning.
> 
> What we have is an object that is conditionally set but not used
> depending on inlining context.  We've generally "allowed" inlining to
> expose new warnings in the sense that inlining may (for example) allow
> us to remove the addressibility property on an object -- which makes the
> object subject to the usual -Wuninitialized analysis.  In fact, I think
> we've generally considered that a positive outcome because it's exposing
> bugs in subtle paths.

But set-but-not-used isn't a warning like that, it's a warning like
'unused variable' which directs the user to simply delete the
affected stmts (and variable).  So the warning should only trigger
if that would not make the program fail to compile.

> I'm less sure that this case falls into that same category.  What we're
> really talking about is warning for a partially dead store.   Would we
> want a warning if rather than a memset this was a simple store?   Is
> that the right guiding principle here?

We had a -Wunreachable-code that was quite useless because it basically
triggered at DCE so was full of useless false positives.  It was merely
an optimization report.  I fear this one will be similar
(warning: we applied DSE!).

> I hate to say it, but I wonder if this is another case where there
> likely won't be a clear consensus and we're going to end up with a two
> level warning system?
> 
> For something like Martin's case what I really think we should do is
> sink the memset call into the conditional.  In cases where "i" is not a
> constant, but actually has the value zero at runtime we win.
> 
> --
> 
> So I've got no objections to the idea of using DSE to detect the dead
> store and potentially warn.  My concern is are we in a case where that
> warning is going to annoy users and we end up needing a level of
> -Wunused-but-set.

It's not a warning.  It's an hint for an optimization the user could
apply (sink the thing!) or a report for an optimization we do.

Do not go down the route of -Wunreachable-code again please.

Richard.

> Jeff
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)