[PATCH v3] pass: Run cleanup passes before SLP [PR96789]

Kewen.Lin linkw@linux.ibm.com
Tue Nov 3 06:38:49 GMT 2020


Hi Richard,

Thanks again for your review!

on 2020/11/2 下午6:23, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>> diff --git a/gcc/function.c b/gcc/function.c
>> index 2c8fa217f1f..3e92ee9c665 100644
>> --- a/gcc/function.c
>> +++ b/gcc/function.c
>> @@ -4841,6 +4841,8 @@ allocate_struct_function (tree fndecl, bool abstract_p)
>>       binding annotations among them.  */
>>    cfun->debug_nonbind_markers = lang_hooks.emits_begin_stmt
>>      && MAY_HAVE_DEBUG_MARKER_STMTS;
>> +
>> +  cfun->pending_TODOs = 0;
> 
> The field is cleared on allocation.  I think it would be better
> to drop this, to avoid questions about why other fields aren't
> similarly zero-initialised.
> 
>>  }
>>  
>>  /* This is like allocate_struct_function, but pushes a new cfun for FNDECL
>> diff --git a/gcc/function.h b/gcc/function.h
>> index d55cbddd0b5..ffed6520bf9 100644
>> --- a/gcc/function.h
>> +++ b/gcc/function.h
>> @@ -269,6 +269,13 @@ struct GTY(()) function {
>>    /* Value histograms attached to particular statements.  */
>>    htab_t GTY((skip)) value_histograms;
>>  
>> +  /* Different from normal TODO_flags which are handled right at the
>> +     begin or the end of one pass execution, the pending_TODOs are
> 
> beginning
> 
>> +     passed down in the pipeline until one of its consumers can
>> +     perform the requested action.  Consumers should then clear the
>> +     flags for the actions that they have taken.  */
>> +  unsigned int pending_TODOs;
>> +
>>    /* For function.c.  */
>>  
>>    /* Points to the FUNCTION_DECL of this function.  */
>> […]
>> diff --git a/gcc/tree-ssa-loop-ivcanon.c b/gcc/tree-ssa-loop-ivcanon.c
>> index 298ab215530..9a9076cee67 100644
>> --- a/gcc/tree-ssa-loop-ivcanon.c
>> +++ b/gcc/tree-ssa-loop-ivcanon.c
>> @@ -1411,6 +1411,13 @@ tree_unroll_loops_completely_1 (bool may_increase_size, bool unroll_outer,
>>  	  bitmap_clear (father_bbs);
>>  	  bitmap_set_bit (father_bbs, loop_father->header->index);
>>  	}
>> +      else if (unroll_outer
>> +	       && !(cfun->pending_TODOs
>> +		    & PENDING_TODO_force_next_scalar_cleanup))
>> +	{
>> +	  /* Trigger scalar cleanup once any outermost loop gets unrolled.  */
>> +	  cfun->pending_TODOs |= PENDING_TODO_force_next_scalar_cleanup;
>> +	}
> 
> I can see it would make sense to test whether the flag is already set
> if we were worried about polluting the cache line.  But this test and
> set isn't performance-sensitive, so I think it would be clearer to
> remove the “&& …” part of the condition.
> 
> Nit: there should be no braces around the block, since it's a single
> statement.
> 
> OK with those changes, thanks.

The patch was updated as your comments above, re-tested on Power8
and committed in r11-4637.

BR,
Kewen


More information about the Gcc-patches mailing list