Re: Make note_stores take an rtx_insn

[Jeff Law <law at redhat dot com>]

On 9/9/19 5:33 AM, Richard Sandiford wrote:
> I have a series of patches that (as a side effect) makes all rtl
> passes use the information collected by -fipa-ra.  This showed up a
> latent bug in the liveness tracking in regrename.c, which doesn't take
> CALL_INSN_FUNCTION_USAGE into account when processing clobbers.
> 
> This actually seems to be quite a common problem with passes that use
> note_stores; only a handful remember to walk CALL_INSN_FUNCTION_USAGE
> too.  I think it was just luck that I saw it with regrename first.>
> This patch tries to make things more robust by passing an insn rather
> than a pattern to note_stores.  The old function is still available
> as note_pattern_stores for the few places that need it.
> 
> When updating callers, I've erred on the side of using note_stores
> rather than note_pattern_stores, because IMO note_stores should be
> the default choice and we should only use note_pattern_stores if
> there's a specific reason.  Specifically:
> 
> * For cselib.c, "body" may be a COND_EXEC_CODE instead of the main
>   insn pattern.
> 
> * For ira.c, I wasn't sure whether extending no_equiv to
>   CALL_INSN_FUNCTION_USAGE really made sense, since we don't do that
>   for normal call-clobbered registers.  Same for mark_not_eliminable
>   in reload1.c
> 
> * Some other places only have a pattern available, and since those
>   places wouldn't benefit from walking CALL_INSN_FUNCTION_USAGE,
>   it seemed better to alter the code as little as possible.
> 
> * In the config/ changes, quite a few callers have already weeded
>   out CALL insns.  It still seemed better to use note_stores rather
>   than prematurely optimise.  (note_stores should tail call to
>   note_pattern_stores once it sees that the insn isn't a call.)
> 
> The patch also documents what SETs mean in CALL_INSN_FUNCTION_USAGE.
> 
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  Also tested by
> building one target for each CPU directory and testing for no new
> warnings.  Comparing the asm output for gcc.c-torture, gcc.dg and
> g++.dg for those targets at -O2 showed some changes for AArch64
> and AArch32 targets (all neutral or very minor improvements).
> There were no changes for other ports.
> 
> (This was before BPF, but fortunately BPF doesn't use note_stores. :-))
> 
> OK to install?
> 
> Richard
> 
> 
> 2019-09-09  Richard Sandiford  <richard.sandiford@arm.com>
> 
> gcc/
> 	* rtl.h (CALL_INSN_FUNCTION_USAGE): Document what SETs mean.
> 	(note_pattern_stores): Declare.
> 	(note_stores): Take an rtx_insn *.
> 	* rtlanal.c (set_of): Use note_pattern_stores instead of note_stores.
> 	(find_all_hard_reg_sets): Pass the insn rather than its pattern to
> 	note_stores.  Remove explicit handling of CALL_INSN_FUNCTION_USAGE.
> 	(note_stores): Take an rtx_insn * as argument and process
> 	CALL_INSN_FUNCTION_USAGE.  Rename old function to...
> 	(note_pattern_stores): ...this.
> 	(find_first_parameter_load): Pass the insn rather than
> 	its pattern to note_stores.
> 	* alias.c (memory_modified_in_insn_p, init_alias_analysis): Likewise.
> 	* caller-save.c (setup_save_areas, save_call_clobbered_regs)
> 	(insert_one_insn): Likewise.
> 	* combine.c (combine_instructions): Likewise.
> 	(likely_spilled_retval_p): Likewise.
> 	(try_combine): Use note_pattern_stores instead of note_stores.
> 	(record_dead_and_set_regs): Pass the insn rather than its pattern
> 	to note_stores.
> 	(reg_dead_at_p): Likewise.
> 	* config/bfin/bfin.c (workaround_speculation): Likewise.
> 	* config/c6x/c6x.c (maybe_clobber_cond): Likewise.  Take an rtx_insn *
> 	rather than an rtx.
> 	* config/frv/frv.c (frv_registers_update): Use note_pattern_stores
> 	instead of note_stores.
> 	(frv_optimize_membar_local): Pass the insn rather than its pattern
> 	to note_stores.
> 	* config/gcn/gcn.c (gcn_md_reorg): Likewise.
> 	* config/i386/i386.c (ix86_avx_u128_mode_after): Likewise.
> 	* config/mips/mips.c (vr4130_true_reg_dependence_p): Likewise.
> 	(r10k_needs_protection_p, mips_sim_issue_insn): Likewise.
> 	(mips_reorg_process_insns): Likewise.
> 	* config/s390/s390.c (s390_regs_ever_clobbered): Likewise.
> 	* config/sh/sh.c (flow_dependent_p): Likewise.  Take rtx_insn *s
> 	rather than rtxes.
> 	* cse.c (delete_trivially_dead_insns): Pass the insn rather than
> 	its pattern to note_stores.
> 	* cselib.c (cselib_record_sets): Use note_pattern_stores instead
> 	of note_stores.
> 	* dce.c (mark_nonreg_stores): Remove the "body" parameter and pass
> 	the insn to note_stores.
> 	(prescan_insns_for_dce): Update call accordingly.
> 	* ddg.c (mem_write_insn_p): Pass the insn rather than its pattern
> 	to note_stores.
> 	* df-problems.c (can_move_insns_across): Likewise.
> 	* dse.c (emit_inc_dec_insn_before, replace_read): Likewise.
> 	* function.c (assign_parm_setup_reg): Likewise.
> 	* gcse-common.c (record_last_mem_set_info_common): Likewise.
> 	* gcse.c (load_killed_in_block_p, compute_hash_table_work): Likewise.
> 	(single_set_gcse): Likewise.
> 	* ira.c (validate_equiv_mem): Likewise.
> 	(update_equiv_regs): Use note_pattern_stores rather than note_stores
> 	for no_equiv.
> 	* loop-doloop.c (doloop_optimize): Pass the insn rather than its
> 	pattern to note_stores.
> 	* loop-invariant.c (calculate_loop_reg_pressure): Likewise.
> 	* loop-iv.c (simplify_using_initial_values): Likewise.
> 	* mode-switching.c (optimize_mode_switching): Likewise.
> 	* optabs.c (emit_libcall_block_1): Likewise.
> 	(expand_atomic_compare_and_swap): Likewise.
> 	* postreload-gcse.c (load_killed_in_block_p): Likewise.
> 	(record_opr_changes): Likewise.  Remove explicit handling of
> 	CALL_INSN_FUNCTION_USAGE.
> 	* postreload.c (reload_combine, reload_cse_move2add): Likewise.
> 	* regcprop.c (kill_clobbered_values): Likewise.
> 	(copyprop_hardreg_forward_1): Pass the insn rather than its pattern
> 	to note_stores.
> 	* regrename.c (build_def_use): Likewise.
> 	* reload1.c (reload):  Use note_pattern_stores instead of note_stores
> 	for mark_not_eliminable.
> 	(reload_as_needed): Pass the insn rather than its pattern
> 	to note_stores.
> 	(emit_output_reload_insns): Likewise.
> 	* resource.c (mark_target_live_regs): Likewise.
> 	* sched-deps.c (init_insn_reg_pressure_info): Likewise.
> 	* sched-rgn.c (sets_likely_spilled): Use note_pattern_stores
> 	instead of note_stores.
> 	* shrink-wrap.c (try_shrink_wrapping): Pass the insn rather than
> 	its pattern to note_stores.
> 	* stack-ptr-mod.c (pass_stack_ptr_mod::execute): Likewise.
> 	* var-tracking.c (adjust_insn, add_with_sets): Likewise.
Kind of surprising how pervasive this problem was.  As you said, just
happened to hit it in regrename first...

If you can get it in Mon/Tues, then my tester will fully spin it prior
to Cauldron...



OK for the trunk.

Jeff