From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id 3EF15385840A for ; Thu, 11 Nov 2021 12:07:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3EF15385840A Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 11F862827EE; Thu, 11 Nov 2021 13:07:15 +0100 (CET) Date: Thu, 11 Nov 2021 13:07:15 +0100 From: Jan Hubicka To: Richard Biener Cc: GCC Patches Subject: Re: Use modref summary to DSE calls to non-pure functions Message-ID: <20211111120715.GC47134@kam.mff.cuni.cz> References: <20211110124316.GF97553@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Nov 2021 12:07:18 -0000 > > + /* Unlike alias oracle we can not skip subtrees based on TBAA check. > > + Count the size of the whole tree to verify that we will not need too many > > + tests. */ > > + FOR_EACH_VEC_SAFE_ELT (summary->stores->bases, i, base_node) > > + FOR_EACH_VEC_SAFE_ELT (base_node->refs, j, ref_node) > > + FOR_EACH_VEC_SAFE_ELT (ref_node->accesses, k, access_node) > > + if (num_tests++ > max_tests) > > + return false; > > at least the innermost loop can be done as > > if (num_tests += ref_node->accesses.length () > max_tests) > > no? Yep that was stupid, sorry for that ;)) > > > + > > + /* Walk all memory writes and verify that they are dead. */ > > + FOR_EACH_VEC_SAFE_ELT (summary->stores->bases, i, base_node) > > + FOR_EACH_VEC_SAFE_ELT (base_node->refs, j, ref_node) > > + FOR_EACH_VEC_SAFE_ELT (ref_node->accesses, k, access_node) > > + { > > + /* ??? if offset is unkonwn it may be negative. Not sure > > + how to construct ref here. */ > > I think you can't, you could use -poly_int64_max or so. I need a ref to give to dse_classify_store. It needs base to track live bytes etc which is not very useful if I do not know the range. However DSE is still useful since I can hit free or end of lifetime of the decl. I was wondering if I should simply implement a lightweight version of dse_clasify_store that handles this case? > > > + if (!access_node->parm_offset_known) > > + return false; > > But you could do this check in the loop computing num_tests ... > (we could also cache the count and whether any of the refs have unknown offset > in the summary?) Yep, I plan to add cache for bits like this (and the check for accessing global memory). Just want to push bit more of the cleanups I have in my local tree. > > > + tree arg; > > + if (access_node->parm_index == MODREF_STATIC_CHAIN_PARM) > > + arg = gimple_call_chain (stmt); > > + else > > + arg = gimple_call_arg (stmt, access_node->parm_index); > > + > > + ao_ref ref; > > + poly_offset_int off = (poly_offset_int)access_node->offset > > + + ((poly_offset_int)access_node->parm_offset > > + << LOG2_BITS_PER_UNIT); > > + poly_int64 off2; > > + if (!off.to_shwi (&off2)) > > + return false; > > + ao_ref_init_from_ptr_and_range > > + (&ref, arg, true, off2, access_node->size, > > + access_node->max_size); > > + ref.ref_alias_set = ref_node->ref; > > + ref.base_alias_set = base_node->base; > > + > > + bool byte_tracking_enabled > > + = setup_live_bytes_from_ref (&ref, live_bytes); > > + enum dse_store_status store_status; > > + > > + store_status = dse_classify_store (&ref, stmt, > > + byte_tracking_enabled, > > + live_bytes, &by_clobber_p); > > + if (store_status != DSE_STORE_DEAD) > > + return false; > > + } > > + /* Check also value stored by the call. */ > > + if (gimple_store_p (stmt)) > > + { > > + ao_ref ref; > > + > > + if (!initialize_ao_ref_for_dse (stmt, &ref)) > > + gcc_unreachable (); > > + bool byte_tracking_enabled > > + = setup_live_bytes_from_ref (&ref, live_bytes); > > + enum dse_store_status store_status; > > + > > + store_status = dse_classify_store (&ref, stmt, > > + byte_tracking_enabled, > > + live_bytes, &by_clobber_p); > > + if (store_status != DSE_STORE_DEAD) > > + return false; > > + } > > + delete_dead_or_redundant_assignment (gsi, "dead", need_eh_cleanup); > > + return true; > > +} > > + > > namespace { > > > > const pass_data pass_data_dse = > > @@ -1235,7 +1363,14 @@ pass_dse::execute (function *fun) > > gimple *stmt = gsi_stmt (gsi); > > > > if (gimple_vdef (stmt)) > > - dse_optimize_stmt (fun, &gsi, live_bytes); > > + { > > + gcall *call = dyn_cast (stmt); > > + > > + if (call && dse_optimize_call (&gsi, live_bytes)) > > + /* We removed a dead call. */; > > + else > > + dse_optimize_store (fun, &gsi, live_bytes); > > I think we want to refactor both functions, dse_optimize_stmt has some > early outs that apply generally, and it handles some builtin calls > that we don't want to re-handle with dse_optimize_call. > > So I wonder if it is either possible to call the new function from > inside dse_optimize_stmt instead, after we handled the return > value of call for example or different refactoring can make the flow > more obvious. It was my initial plan. However I was not sure how much I would get from that. The function starts with: /* Don't return early on *this_2(D) ={v} {CLOBBER}. */ if (gimple_has_volatile_ops (stmt) && (!gimple_clobber_p (stmt) || TREE_CODE (gimple_assign_lhs (stmt)) != MEM_REF)) return; ao_ref ref; if (!initialize_ao_ref_for_dse (stmt, &ref)) return; The check about clobber does not apply to calls and then it gives up on functions not returning aggregates (that is a common case). For functions returing aggregates it tries to prove that retval is dead and replace it. I guess I can simply call my analysis from the second return above and from the code removing dead LHS call instead of doing it from the main walker and drop the LHS handling? Thank you, Honza > > Thanks, > Richard. > > > + } > > else if (def_operand_p > > def_p = single_ssa_def_operand (stmt, SSA_OP_DEF)) > > {