Re: [PATCH] PR91195: fix -Wmaybe-uninitialized warning for conditional store optimization

[Jeff Law <law at redhat dot com>]

On 7/29/19 9:50 AM, Jakub Jelinek wrote:
> On Tue, Jul 23, 2019 at 04:26:24AM +0000, JiangNing OS wrote:
>> --- a/gcc/ChangeLog
>> +++ b/gcc/ChangeLog
>> @@ -1,3 +1,9 @@
>> +2019-07-22  Jiangning Liu  <jiangning.liu@amperecomputing.com>
>> +
>> +	PR middle-end/91195
>> +	* tree-ssa-phiopt.c (cond_store_replacement): Work around
>> +	-Wmaybe-uninitialized warning.
>> +
>>  2019-07-22  Stafford Horne  <shorne@gmail.com>
>>  
>>  	* config/or1k/or1k.c (or1k_expand_compare): Check for int before
>> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
>> index b64bde695f4..7a86007d087 100644
>> --- a/gcc/tree-ssa-phiopt.c
>> +++ b/gcc/tree-ssa-phiopt.c
>> @@ -2240,6 +2240,14 @@ cond_store_replacement (basic_block middle_bb, basic_block join_bb,
>>        tree base = get_base_address (lhs);
>>        if (!auto_var_p (base) || TREE_ADDRESSABLE (base))
>>  	return false;
>> +
>> +      /* The transformation below will inherently introduce a memory load,
>> +	 for which LHS may not be initialized yet if it is not in NOTRAP,
>> +	 so a -Wmaybe-uninitialized warning message could be triggered.
>> +	 Since it's a bit hard to have a very accurate uninitialization
>> +	 analysis to memory reference, we disable the warning here to avoid
>> +	 confusion.  */
>> +      TREE_NO_WARNING (lhs) = 1;
> 
> I don't like this, but not for the reasons Martin stated, we use
> TREE_NO_WARNING not just when we've emitted warnings, but in many places
> when we've done something that might trigger false positives.
> Yes, it would be nice to do it more selectively.
> 
> The problem I see with the above though is that lhs might very well be
> a decl, and setting TREE_NO_WARNING on it then doesn't affect only the
> hoisted load, but also all other code that refers to the decl.
LHS is restricted to just MEM_REF, ARRAY_REF and COMPONENT_REF.  We'd be
setting the NO_WARNING bits on the toplevel expression, but not on
anything shared like a _DECL node.

So what we're losing here would be things like out of bounds array
checks on the LHS, so it still sucks.


> 
> If the TREE_NO_WARNING bit is set on something that isn't shareable, that is
> fine with me, like a MEM_REF, TARGET_MEM_REF or handled component.  If lhs
> is a decl, can we force a MEM_REF around it (and won't we fold it back to
> the decl?)?  Or perhaps better, can we gimple_set_no_warning on the load
> stmt instead?
We have the toplevel statement, so that might be worth a try as well.

jeff