From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 48041 invoked by alias); 9 Sep 2019 18:01:39 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 48032 invoked by uid 89); 9 Sep 2019 18:01:39 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-4.2 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.1 spammy=H*f:sk:de3f586, H*i:sk:de3f586 X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.110.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Sep 2019 18:01:37 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 14B121000; Mon, 9 Sep 2019 11:01:36 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.99.62]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 946D13F71F; Mon, 9 Sep 2019 11:01:35 -0700 (PDT) From: Richard Sandiford To: Jeff Law Mail-Followup-To: Jeff Law ,gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org Subject: Re: Make note_stores take an rtx_insn References: Date: Mon, 09 Sep 2019 18:01:00 -0000 In-Reply-To: (Jeff Law's message of "Mon, 9 Sep 2019 11:55:35 -0600") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00599.txt.bz2 Jeff Law writes: > 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 >> >> 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. Thanks for the reviews! I've now applied this and the HARD_REG_SET stuff. Richard